[VarDumper] Allow seamless use of Data clones#21638
Merged
fabpot merged 1 commit intosymfony:masterfrom Feb 27, 2017
Merged
Conversation
0efda24 to
37a5c2a
Compare
b8bc469 to
bc7d10e
Compare
Member
Author
|
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? |
bc7d10e to
f958481
Compare
Member
|
I did not review the code, but I'm 👍 for the feature. |
e3a2f85 to
f3ea3ab
Compare
stof
reviewed
Feb 20, 2017
| * @var ClonerInterface | ||
| */ | ||
| private $cloner; | ||
| private static $cloner; |
Member
Author
There was a problem hiding this comment.
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 to
9f0a245
Compare
45d6e29 to
f7fa04b
Compare
Member
Author
|
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 |
f7fa04b to
2962092
Compare
2962092 to
ab716c6
Compare
Member
|
Thank you @nicolas-grekas. |
fabpot
added a commit
that referenced
this pull request
Feb 27, 2017
…-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
This was referenced Apr 30, 2017
fabpot
added a commit
that referenced
this pull request
May 1, 2017
…(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/A Unless I missed something, any usage of this property were removed in #21638. Commits ------- 96743e6 [Profiler] DataCollector: Remove unused static property
Merged
nicolas-grekas
added a commit
that referenced
this pull request
May 28, 2017
…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, ref ab716c6#diff-78ea21636d0319e1c804ccc1a33f68f3R271. This breaks the form profiler panel. Commits ------- 5e6b3a5 Fixed options stub values display in form profiler
fabpot
added a commit
that referenced
this pull request
Jun 3, 2017
…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/A Since 3.3, if you inspect your logs when accessing the form profiler panel, you'll see some of these: ```sh php.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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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: