Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[VarDumper] Allow seamless use of Data clones#21638
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
b8bc469 tobc7d10eComparenicolas-grekas commentedFeb 20, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Note that this PR is a follow up of#19986 and#20675. I tried this on Symfony Demo, route Now, if we remove the same limits on master, the profile shows a ~30% increase: Before trying to add some limits again to fit the same performance budget as already on master, could someone try this patch on a form-heavy page? |
lyrixx commentedFeb 20, 2017
I did not review the code, but I'm 👍 for the feature. |
e3a2f85 tof3ea3abCompare| * @var ClonerInterface | ||
| */ | ||
| private$cloner; | ||
| privatestatic$cloner; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why making it static ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
because there is no state in it, and each datacollector is currently instanciating its own cloner, which I measured as a free-to-save overhead (addCasters being the "heavy" loop that takes some time).
41ddec9 to9f0a245Compare45d6e29 tof7fa04bComparenicolas-grekas commentedFeb 27, 2017
Here we are, PR is ready, and Blackfire is great! It allowed me to find nested problems. https://blackfire.io/profiles/compare/afd80c91-2d53-4104-9f4d-7fbc04e86b46/graph |
fabpot commentedFeb 27, 2017
Thank you@nicolas-grekas. |
…-grekas)This PR was merged into the 3.3-dev branch.Discussion----------[VarDumper] Allow seamless use of Data clones| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -By implementing `ArrayAccess`, `Countable`, `IteratorAggregate`, `__get`, `__isset` and `__toString`, VarDumper's `Data` objects become seamless and behave almost identically from their original clones values, especially from the PoV of Twig.In data collectors, this allows replacing the many nested calls to `cloneVar` by a single one.This makes the code simpler, and should make a significant difference in term of performance.Todo:- [x] push a Blackfire profile comparison- [x] double check that the profiler works as expected.Commits-------ab716c6 [VarDumper] Allow seamless use of Data clones
…(ogizanagi)This PR was merged into the 3.3-dev branch.Discussion----------[Profiler] DataCollector: Remove unused static property| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | N/A| License | MIT| Doc PR | N/AUnless I missed something, any usage of this property were removed in#21638.Commits-------96743e6 [Profiler] DataCollector: Remove unused static property
…orm profiler (HeahDude)This PR was merged into the 3.3 branch.Discussion----------[WebProfilerBundle] Fixed options stub values display in form profiler| Q | A| ------------- | ---| Branch? | 3.3| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->| Tests pass? | yes| Fixed tickets | ~| License | MIT| Doc PR | ~Since 3.3 and#21638, serializing form options gives either a `CutStub` instance or a simple var, refab716c6#diff-78ea21636d0319e1c804ccc1a33f68f3R271.This breaks the form profiler panel.Commits-------5e6b3a5 Fixed options stub values display in form profiler
…ons (ogizanagi)This PR was merged into the 3.3 branch.Discussion----------[Form][Profiler] Fixes form collector triggering deprecations| Q | A| ------------- | ---| Branch? | 3.3 <!-- see comment below -->| Bug fix? | yes| New feature? | no <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks? | no| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->| Tests pass? | yes| Fixed tickets | N/A <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | N/ASince 3.3, if you inspect your logs when accessing the form profiler panel, you'll see some of these:```shphp.INFO: User Deprecated: The Symfony\Bundle\WebProfilerBundle\Twig\WebProfilerExtension::dumpValue() method is deprecated since version 3.2 and will be removed in 4.0.[...] at /src/Symfony/Bundle/WebProfilerBundle/Twig/WebProfilerExtension.php:119```The [WebProfilerExtension](https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/WebProfilerBundle/Twig/WebProfilerExtension.php#L73) is still using a `ValueExporter` instance for BC reasons when the $value ins't an instance of `Data` and this BC layer will be removed in 4.0 (so it'll throw an exception/error when trying to use it with something else than a `Data` instance).The issue is since#21638, collectors (including forms one) have been drastically simplified to leverage the "seamless usage of Data clones", which is great!... But there is a slightly different implementation between `Data::seek()` and [`Data::__get()`](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/VarDumper/Cloner/Data.php#L123-L130). There are probably good reasons for this, but it prevents from using classic Twig property access when the underlying data may be a scalar (`null`, `false`, ...).I already spot that while working on the [Validator panel](https://github.com/symfony/symfony/pull/22554/files#diff-deac3c5ce4aa87243093dcd6a3f77a56R84). Perhaps there is a better solution, though.Anyway, current `master` is currently broken, as it still tries to use the `ValueExporter`, which is already removed. And removing the BC layer in `WebProfilerExtension` isn't enough for now. It needs this fix.BTW it also fixes rendering of the concerned inlined-dumps:|Before|After||--|--||<img width="818" alt="screenshot 2017-06-03 a 13 35 25" src="https://cloud.githubusercontent.com/assets/2211145/26753222/01a692e6-4862-11e7-90d5-9cc9e4832648.PNG">|<img width="817" alt="screenshot 2017-06-03 a 13 35 47" src="https://cloud.githubusercontent.com/assets/2211145/26753224/090d5d6c-4862-11e7-87c1-73d5346f602c.PNG">|Commits-------9de898d [Form][Profiler] Fixes form collector triggering deprecations

Uh oh!
There was an error while loading.Please reload this page.
By implementing
ArrayAccess,Countable,IteratorAggregate,__get,__issetand__toString, VarDumper'sDataobjects become seamless and behave almost identically from their original clones values, especially from the PoV of Twig.In data collectors, this allows replacing the many nested calls to
cloneVarby a single one.This makes the code simpler, and should make a significant difference in term of performance.
Todo: