Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged
fabpot merged 1 commit intosymfony:masterfromnicolas-grekas:data-get
Feb 27, 2017

Conversation

@nicolas-grekas
Copy link
Member

@nicolas-grekasnicolas-grekas commentedFeb 16, 2017
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?yes
Tests pass?yes
Fixed tickets-
LicenseMIT
Doc PR-

By implementingArrayAccess,Countable,IteratorAggregate,__get,__isset and__toString, VarDumper'sData 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 tocloneVar by a single one.

This makes the code simpler, and should make a significant difference in term of performance.

Todo:

  • push a Blackfire profile comparison
  • double check that the profiler works as expected.

ogizanagi and chalasr reacted with hooray emojiogizanagi reacted with heart emoji
@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedFeb 20, 2017
edited
Loading

Note that this PR is a follow up of#19986 and#20675.
Note also that in this PR, I removed any limits on the number of collected items. This adds much more details to the nested structures of profiler panels.

I tried this on Symfony Demo, routeadmin_post_new. The new behavior is enligthened by the increase of the number of calls to theAbstractCloner::castObject method. The net result is a ~20% slow down, as we do more:
https://blackfire.io/profiles/compare/1dd13c84-d3be-4697-856d-7e57c381b856/graph

Now, if we remove the same limits on master, the profile shows a ~30% increase:
https://blackfire.io/profiles/compare/43c2e0b8-0675-4a45-93d5-387d561623c6/graph

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?
@gharlan especially since you reported#19978.
Here is the patch against 3.2 to help testing:
3.2...nicolas-grekas:data-get32

@lyrixx
Copy link
Member

I did not review the code, but I'm 👍 for the feature.

* @var ClonerInterface
*/
private$cloner;
privatestatic$cloner;
Copy link
Member

@stofstofFeb 20, 2017
edited
Loading

Choose a reason for hiding this comment

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

why making it static ?

Copy link
MemberAuthor

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).

@nicolas-grekasnicolas-grekasforce-pushed thedata-get branch 2 times, most recently from41ddec9 to9f0a245CompareFebruary 21, 2017 07:40
@nicolas-grekasnicolas-grekasforce-pushed thedata-get branch 3 times, most recently from45d6e29 tof7fa04bCompareFebruary 27, 2017 19:45
@nicolas-grekas
Copy link
MemberAuthor

Here we are, PR is ready, and Blackfire is great! It allowed me to find nested problems.

capture du 2017-02-27 20-46-35

https://blackfire.io/profiles/compare/afd80c91-2d53-4104-9f4d-7fbc04e86b46/graph

HeahDude and xabbuh reacted with hooray emoji

@fabpot
Copy link
Member

Thank you@nicolas-grekas.

@fabpotfabpot merged commitab716c6 intosymfony:masterFeb 27, 2017
fabpot added a commit that referenced this pull requestFeb 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
@nicolas-grekasnicolas-grekas deleted the data-get branchFebruary 27, 2017 22:43
fabpot added a commit that referenced this pull requestMay 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/AUnless I missed something, any usage of this property were removed in#21638.Commits-------96743e6 [Profiler] DataCollector: Remove unused static property
@fabpotfabpot mentioned this pull requestMay 1, 2017
nicolas-grekas added a commit that referenced this pull requestMay 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, refab716c6#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 requestJun 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/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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof left review comments

Assignees

No one assigned

Projects

None yet

Milestone

3.3

Development

Successfully merging this pull request may close these issues.

5 participants

@nicolas-grekas@lyrixx@fabpot@stof@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp