Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PropertyAccess] Remove most ref mismatches to improve perf #18224

Merged
merged 1 commit into from Mar 21, 2016

Conversation

Projects
None yet
6 participants
@nicolas-grekas
Copy link
Member

commented Mar 18, 2016

Q A
Branch? 2.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? no
Fixed tickets -
License MIT
Doc PR -

This PR is for PHP5 where ref mismatches is a perf pain: it removes all ref mismatches along the "getValue" path, and keeps only the required ones on the "setValue" path.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:fix-ref-mismatch branch from df9df89 to ea33a2b Mar 18, 2016

@stof

This comment has been minimized.

Copy link
Member

commented Mar 18, 2016

do you have a profiling comparison of the gain to know how much difference it makes ?

// Save creating references when doing read-only lookups
} elseif (is_array($zval[self::VALUE])) {
$result[self::REF] = &$zval[self::REF][$index];
} elseif (is_object($result[self::VALUE])) {
// Objects are always passed around by reference

This comment has been minimized.

Copy link
@stof

stof Mar 18, 2016

Member

this comment is not accurate (but we don't need a reference when dealing with objects). I suggest updating it to avoid confusing people reading the code

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:fix-ref-mismatch branch 4 times, most recently from 2fceaef to 0c8e7ed Mar 18, 2016

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2016

Here is a first comparison:
https://blackfire.io/profiles/compare/c77e869c-a5ba-44da-8cea-8a45c3c151ab/graph
capture du 2016-03-18 15 10 41

$var = range(1, 1000000);
$pa = new PropertyAccessor();
$res = $pa->getValue($var, '[12]');
@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Mar 18, 2016

A more realistic benchmark would be to use range(1, 1000) or so. In any case, impressive numbers! Thanks for the optimization.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:fix-ref-mismatch branch from 0c8e7ed to 72940d7 Mar 18, 2016

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2016

And one with setValue:
https://blackfire.io/profiles/compare/15688eff-7e1b-48fb-9d97-c6f22278603c/graph
capture du 2016-03-18 15 40 35

$var = range(1, 100000);
$pa = new PropertyAccessor();
$res = $pa->setValue($var, '[12]', $var);
@stof

This comment has been minimized.

Copy link
Member

commented Mar 21, 2016

These numbers are very impressive.

@nicolas-grekas can you also post a benchmark for a deeper property path ?

👍
Status: reviewed

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 21, 2016

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 72940d7 into symfony:2.3 Mar 21, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 21, 2016

bug #18224 [PropertyAccess] Remove most ref mismatches to improve per…
…f (nicolas-grekas)

This PR was merged into the 2.3 branch.

Discussion
----------

[PropertyAccess] Remove most ref mismatches to improve perf

| Q             | A
| ------------- | ---
| Branch?       | 2.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | no
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

This PR is for PHP5 where ref mismatches is a perf pain: it removes all ref mismatches along the "getValue" path, and keeps only the required ones on the "setValue" path.

Commits
-------

72940d7 [PropertyAccess] Remove most ref mismatches to improve perf

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:fix-ref-mismatch branch Mar 21, 2016

@webmozart

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2016

Awesome, thanks @nicolas-grekas :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.