Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

$parse performance/style #8901

Closed
wants to merge 7 commits into from
Closed

$parse performance/style #8901

wants to merge 7 commits into from

Conversation

jbedard
Copy link
Contributor

@jbedard jbedard commented Sep 3, 2014

c074181 might be the only one worth merging to allow the Parser/Lexer objects to be GCed.

f4ea3c9 addresses a TODO although I didn't notice any major improvements (even though the removed wrapper method is often ~10% when profiling...). Unfortunately the wrapper method is still needed for the expressions returned from $parse, but use of getterFns within other expressions don't have the wrapper. This also required a minor error message change.

f4ea3c9 could be expanded in other ways though. Maybe allowing $parseed methods to return something such as a $$watchFn property that can be watched instead of the full expression being watched. The simple case would be returning the raw getterFn instead of the wrapper for simple foo.bar expressions. A more complicated case would be expressions such as myVar % 2 === 0 returning only the myVar getterFn to be watched (and then a special watch delegate to see if the full expression actually changed, which makes it more complicated...). Or only watching the input+args to filters etc. Not sure how common or useful those would be though...

The rest are mostly style/simplifying but I thought I'd leave them in there.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
…ions
@btford
Copy link
Contributor

btford commented Sep 3, 2014

@jbedard thanks for the PR! can you add a benchmark to https://github.com/angular/angular.js/tree/master/benchmarks to prove that these changes are beneficial?

@btford btford added this to the 1.3.0-rc.2 milestone Sep 3, 2014
@jbedard jbedard force-pushed the parse-perf branch 2 times, most recently from 05ee69e to eb47e91 Compare September 4, 2014 15:57

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@jbedard
Copy link
Contributor Author

jbedard commented Sep 5, 2014

@btford I've added a new benchpress benchmark for execution of $parse()ed expressions (fd45e8a). It currently has tests for the different types of expressions (property, binary op, filter, ...). Basically it generates data (2000 rows right now), ng-repeats a bunch of expressions (12 per test right now, except object literals that are too slow) and then measures the digest (x50 right now) time.

I updated eb47e91 such that the wrapper function is now only needed for one-time binding so $parse()ed properties now benefit form this as well. (A wrapper is needed for one-time because the one-time :: is not actually part of parsing so both ::foo and foo now return the exact same getterFn result, but they have different $$watchDelegates so they need to be different functions returned from $parse).

Here are the numbers (ms) for the current tests in the benchmark (12 expressions x 2000 rows x 50 digests):

Test Old New Explanation
simple properties 110 94 needs a few runs to stabilize but seems consistent
field accessors 193 178 $parsePathGetter was ~5% in a quick profile which matches the numbers
field indexors 1160 1160 $parsePathGetter was negligible compared to ensureSafeObject + $parseObjectIndex
prop/operators 206 186 $parsePathGetter was ~8% in a quick profile which matches the numbers
prop/filters 328 326 $parsePathGetter was negligible compared to $parseFilter + pipe operator + binaryFn
prop/literals/func 559 522 $parsePathGetter was mostly negligible compared to $parseFunctionCall + .apply + ensureSafeObject
prop/literals/{} 1474 1440 $parsePathGetter was negligible since it is mainly constants in the object, the .length seems to make the minor difference
prop/literals/[] 1003 979 (same as object literal)

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@jbedard jbedard mentioned this pull request Sep 5, 2014
IgorMinar pushed a commit that referenced this pull request Sep 7, 2014

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Part of #8901
IgorMinar pushed a commit that referenced this pull request Sep 7, 2014

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Part of #8901
IgorMinar pushed a commit that referenced this pull request Sep 7, 2014

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Part of #8901
IgorMinar pushed a commit that referenced this pull request Sep 7, 2014

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Part of #8901
IgorMinar pushed a commit that referenced this pull request Sep 7, 2014

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
…ions

This allows the parser and lexer objects to get GC-ed once the expression
is parsed.

Part of #8901
@IgorMinar IgorMinar self-assigned this Sep 8, 2014
IgorMinar pushed a commit to IgorMinar/angular.js that referenced this pull request Sep 9, 2014

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Part of angular#8901
IgorMinar pushed a commit to IgorMinar/angular.js that referenced this pull request Sep 9, 2014

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Closes angular#8901
IgorMinar pushed a commit to IgorMinar/angular.js that referenced this pull request Sep 9, 2014

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Part of angular#8901
IgorMinar pushed a commit to IgorMinar/angular.js that referenced this pull request Sep 9, 2014

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Closes angular#8901
IgorMinar pushed a commit to IgorMinar/angular.js that referenced this pull request Sep 9, 2014

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Closes angular#8901
IgorMinar pushed a commit to IgorMinar/angular.js that referenced this pull request Sep 9, 2014

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Closes angular#8901
IgorMinar pushed a commit to IgorMinar/angular.js that referenced this pull request Sep 9, 2014

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Closes angular#8901
IgorMinar pushed a commit that referenced this pull request Sep 9, 2014

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Part of #8901
@IgorMinar IgorMinar closed this in b3b476d Sep 9, 2014
@jbedard
Copy link
Contributor Author

jbedard commented Sep 9, 2014

Thanks for looking into all of these and getting them in so fast! And note my comment regarding the changed tests/error-message if you haven't already (can avoid changing the tests/message if we want, see jbedard@cec3344)

mgallag pushed a commit to mgallag/angular.js that referenced this pull request Sep 10, 2014

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Part of angular#8901
mgallag pushed a commit to mgallag/angular.js that referenced this pull request Sep 10, 2014

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Closes angular#8901
mgallag pushed a commit to mgallag/angular.js that referenced this pull request Sep 10, 2014

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Part of angular#8901
mgallag pushed a commit to mgallag/angular.js that referenced this pull request Sep 10, 2014

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Closes angular#8901
mgallag pushed a commit to mgallag/angular.js that referenced this pull request Sep 10, 2014

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Part of angular#8901
mgallag pushed a commit to mgallag/angular.js that referenced this pull request Sep 10, 2014

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Closes angular#8901
ggershoni pushed a commit to ggershoni/angular.js that referenced this pull request Sep 29, 2015

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Part of angular#8901
ggershoni pushed a commit to ggershoni/angular.js that referenced this pull request Sep 29, 2015

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Closes angular#8901
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants