Skip to content

Conversation

@spawnia
Copy link
Collaborator

@spawnia spawnia commented Nov 6, 2025

Summary

This PR optimizes memory usage when using Deferred objects by implementing incremental queue processing. This reduces peak memory usage by ~70% when handling thousands of Deferred objects.

Problem

Issue #972 reported excessive memory usage with Deferred objects. Investigation revealed:

  • 4000 Deferred objects consumed ~54MB peak memory (6.75x baseline of 8MB)
  • This was NOT a memory leak - memory was properly released between requests
  • The issue was peak memory usage during execution

Root Cause

The static SplQueue in SyncPromise accumulated ALL closures before processing. With 4000 Deferred objects, thousands of closures were queued simultaneously, causing high memory usage.

Solution

Implemented incremental queue processing with a batch threshold:

  • Added QUEUE_BATCH_SIZE constant (500 items)
  • When queue exceeds threshold, immediately process a batch
  • This keeps queue size bounded and reduces peak memory

Results

Peak memory usage:

  • Before optimization: ~54MB (6.75x baseline)
  • After optimization: ~16MB (2x baseline)
  • Reduction: 70%

Testing

Added testDeferredMemoryEfficiency() test that:

  • ✅ PASSES with optimization (16MB < 25MB threshold)
  • ❌ FAILS without optimization (48MB > 25MB threshold)
  • Prevents future regressions
  • All 240 existing tests pass

Verification

Tested against original reproduction case from https://github.com/Danbka/php-graphql-deffred showing identical improvements.

Fixes #972

Fixes #972

## Problem

When using Deferred with large datasets (4000+ items), peak memory usage
was excessive:
- WITHOUT Deferred: 8MB peak memory
- WITH Deferred: 54MB peak memory
- **Overhead: 6.75x increase (46MB excess)**

This caused out-of-memory errors in production environments with
constrained memory limits.

## Investigation & Findings

Initial hypothesis was that closures were leaking memory through captured
references. However, testing showed:
1. Memory WAS being released after execution (no leak)
2. Peak memory during execution was the actual problem
3. The issue was architectural, not a reference cleanup problem

Root cause: The static `SplQueue` in `SyncPromise::getQueue()` accumulated
ALL 4000+ closures before `runQueue()` processed them. Each Deferred
object creates a closure that's queued immediately, resulting in thousands
of closures held in memory simultaneously.

## Solution

Implement **incremental queue processing** with a batch size threshold:
- When queue size exceeds 500 items, process a batch immediately
- This keeps the queue size bounded, reducing peak memory
- Processing happens during promise creation rather than only at the end
- No API changes required - fully backward compatible

## Results

Memory usage with 4000 Deferred objects:
- **Before:** 54MB peak (6.75x baseline)
- **After:** 16MB peak (2x baseline)
- **Improvement: 70% reduction in peak memory usage**

## Implementation

1. Added `QUEUE_BATCH_SIZE` constant (500 items)
2. Added `processBatch()` method to process queue incrementally
3. Added threshold checks after `enqueue()` operations to trigger batching
4. Added `testDeferredMemoryEfficiency()` with proper verification:
   - **PASSES** with optimization: 16MB < 25MB threshold ✓
   - **FAILS** without optimization: 48MB > 25MB threshold ✗

All existing tests pass (240 tests, 1196 assertions).

## Testing Methodology

We verified this fix using the original reproduction case from
https://github.com/Danbka/php-graphql-deffred:
- Tested with actual 4000-item dataset
- Measured peak memory with and without optimization
- Confirmed 70% memory reduction
- Ensured no performance regression

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@spawnia spawnia requested a review from ruudk November 6, 2025 10:22
Copy link
Collaborator

@ruudk ruudk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having a hard time understanding it. But since you measured it and have it tested, I would say let's go!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Using Deferred produces memory leak?

3 participants