fix: prevent unbounded memory growth in Reflection::function() cache

[MaelitoP]

Fixes #800

Problem

Reflection::function() uses spl_object_hash($closure) as cache key. Since Closure::fromCallable() creates a new object each time, spl_object_hash returns a unique hash per call — even when closures reference the same source function.

The static $functionReflection array grows unboundedly, leaking per TreeMapper::map() invocation and causing OOM in long-running processes.

Fix

Replace the cache key with fileName:startLine for user-defined functions and the function name for built-in functions. This is stable across Closure instances and bounded by the number of unique function definitions — matching the strategy already used by InMemoryFunctionDefinitionRepository.

Trade-off: A ReflectionFunction is now always instantiated to compute the cache key, even on hits. However, ReflectionFunction construction is cheap and the previous implementation had an effective 0% cache hit rate (unique keys on every call), so this is strictly better.

[romm]

Hi @MaelitoP, thanks for the PR and the detailed report.

Honestly the memoized cache does not seem to be useful anymore, as we instantiate ReflectionFunction everytime, so I'd suggest to replace every call to \CuyZ\Valinor\Utility\Reflection\Reflection::function with new ReflectionFunction directly (and then we can safely delete the former one). Could you do it and run your benchmark again on the codebase where you detected the issue?

[MaelitoP]

Hi @romm, thanks for the review!

I tested your suggestion. Removing Reflection::function() entirely and inlining new ReflectionFunction(Closure::fromCallable(...)) at every call site. Here are the benchmark results compared to the original bug and the file:line cache key approach:

Metric	Baseline (bug)	file:line cache (this PR)	No cache (your suggestion)
Avg delta per map() call	+665 KB	+181 KB	+355 KB
Total growth (10 min run)	+20 MB	-5.9 MB (shrinking)	+8.1 MB (still growing)
Projected OOM (256 MB)	~85 min	never	much later, but still grows

Removing the cache entirely is a ~50% improvement over the bug, but the file:line cache key achieves ~73% improvement and actually makes memory shrink over time (GC reclaims more than what's allocated).

The cache still provides value: it prevents redundant ReflectionFunction creation across repeated calls with the same source function. Without it, new objects are created every time and some are retained through other code paths (e.g. InMemoryFunctionDefinitionRepository::forCallable()).

The trade-off with the current file:line approach is that we always create a ReflectionFunction to compute the key — but since the previous spl_object_hash approach had an effective 0% cache hit rate, this is strictly better in practice.

Let me know how you'd like to proceed!

[MaelitoP]

Hi @romm,

Any news ?

[romm]

Hi @MaelitoP, sorry for not coming back to you, I've been busy lately. 🙂

There's still something that confuses me, with the changes in this PR, the method looks like this:

    public static function function(callable $function): ReflectionFunction
    {
        $closure = Closure::fromCallable($function);
        $reflection = new ReflectionFunction($closure);

        // @infection-ignore-all / We don't need to test the cache key strategy
        $fileName = $reflection->getFileName();

        // @infection-ignore-all / Built-in functions have no file; use the function name as key instead.
        $key = $fileName !== false
            ? $fileName . ':' . $reflection->getStartLine()
            : $reflection->getName();

        return self::$functionReflection[$key] ??= $reflection;
    }

So, everytime we call it, we do instantiate ReflectionFunction, so the cache really seems useless in this case. I don't get how directly instantiating ReflectionFunction everywhere can be slower/use more memory. 🤔

Any chance your benchmark was not running on the correct version?

[MaelitoP]

Any chance your benchmark was not running on the correct version?

I will double check and I come back to you !