[VarExporter] optimize dumped code in time and space #28295
Conversation
3caf702 to
dca5f70
Compare
| }; | ||
|
|
||
| if ($propertyReflectors) { | ||
| self::$configurators[$class] = \Closure::bind(self::$configurators[$class], null, eval('return new class() extends '.$class.'{};')); |
There was a problem hiding this comment.
It is common for hosts to disable eval for security reasons. Is it possible to keep the less optimized code for such case (checked using function_exists('eval')) ? Or maybe to avoid using eval entirely ?
There was a problem hiding this comment.
eval is not a function, it's a language construct
how can you disable it?
There was a problem hiding this comment.
hmm, indeed, disabling it is not possible through php.ini. It was the suhosin extension disabling it. Not sure how this gets detected then
There was a problem hiding this comment.
I actually figured out a way to not need eval() and still make the dump more compact are fast.
| ]), | ||
| $r = \Symfony\Component\VarExporter\Internal\Registry::class, | ||
| $o = [ | ||
| clone (($p =& $r::$prototypes)[\DateTime::class] ?? $r::p(\DateTime::class, true)), |
There was a problem hiding this comment.
why not keeping \Symfony\Component\VarExporter\Internal\Registry::$prototypes ? Is there any benefit to making the class name a variable ?
There was a problem hiding this comment.
accessing a local var is slightly, and reduces the size of the export as a bonus
There was a problem hiding this comment.
I was talking about $r, not about $p
There was a problem hiding this comment.
Oh sorry. It's purely for compactness, without any performance impact.
There was a problem hiding this comment.
I removed the$r alias, it's not useful in the end.
9150a9c to
c467ec2
Compare
|
comments addressed, thanks @stof |
| $seen = array(); | ||
| $prototypesAccess = 0; | ||
| $factoriesAccess = 0; | ||
| $r = '\\'.Registry::class; |
There was a problem hiding this comment.
I suggest avoiding the usage of a one-letter variable here to make the code more readable (that code is not generated code).
Or maybe even not using it, as was done before (which would probably even make OPCache resolve some of the concatenations at compile-time)
There was a problem hiding this comment.
I've been considering this, but honestly, it would make lines longer and less readable to me.
I removed some but I'd prefer leaving the remaining ones as is.
| @@ -37,8 +37,6 @@ public function __construct(?Registry $registry, ?Values $values, array $propert | |||
|
|
|||
| public static function pop($objects, $values, $properties, $value, $wakeups) | |||
There was a problem hiding this comment.
should this method still be called pop now ? It is not popping the stack anymore.
There was a problem hiding this comment.
true, renamed to Hydrator::hydrate()
8635f03 to
16bbdfc
Compare
|
I split the |
16bbdfc to
07e90d7
Compare
|
Thank you @nicolas-grekas. |
…colas-grekas) This PR was merged into the 4.2-dev branch. Discussion ---------- [VarExporter] optimize dumped code in time and space | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Let's squeeze some more µs when running exported code. On a simple case run 100k times with a few objects, I go from 1.8s to 1.5s. The generated exports are also a bit smaller if it matters. This works by: - using local variables instead of manually dealing with a stack - creating more optimized object hydrators for internal classes This PR also fixes handling of hard references that are bound to external variables. Commits ------- 07e90d7 [VarExporter] optimize dumped code in time and space
Let's squeeze some more µs when running exported code.
On a simple case run 100k times with a few objects, I go from 1.8s to 1.5s.
The generated exports are also a bit smaller if it matters.
This works by:
This PR also fixes handling of hard references that are bound to external variables.