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

perf($compile): wrap try/catch of collect comment directives into a function to avoid V8 deopt #14848

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
7 participants
@drpicox
Copy link
Contributor

commented Jun 30, 2016

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...) its a performance optimization. It turns out that try/catch statements prevents the optimization of the function collectDirectives, which has a great weight in the initial document parse.

What is the current behavior? (You can also link to an open issue here)
It should be around a 5% faster in the angular initial document parse, and also in directives compilation.

What is the new behavior (if this is a feature change)?
There is no change in the functionality.

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

Other information:
See: GoogleChrome/devtools-docs#53

I did all performance checking over existing code from my customers, in their code this optimization makes it go a 5% faster in the initial parse time and compile time in average (I just performed 200 evaluation comparison with execution times around 50 - 100ms of each evaluation). I did the performance test over chrome, safari and firefox, all of them with similar speedup results.
I am not sure which environment do you use to check performance (I think that it is probably related to benchpress). ¿Do you have a guide about how to set up and do testing?

I found the problem while doing performance optimization in a customer code. Just clicking around I found this message lots of times:
screen shot 2016-06-29 at 21 44 27

screen shot 2016-06-29 at 21 46 24

It should solve it, because in production environments usually there are no comments, so, this function is likely to be never executed.

I did no new documentation neither new tests because nothing changes but performance.

@Narretz Narretz added the type: perf label Jun 30, 2016

@Narretz Narretz added this to the 1.5.x milestone Jun 30, 2016

@Narretz

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2016

Great PR, thanks for this. Could you add a comment to the function that indicates that this is for perf reasons?
We have benchpress, but it's very coarse atm, so I don't know if it'll show usable numbers. If you cloned and build angular, you can run the included benchmarks like this:

There is a configured grunt task for building the benchmarks,
grunt bp_build, which places the runnable benchmarks in "/build/benchmarks/".
The existing grunt webserver task can be used to serve the built benchmarks at localhost:8000/build/benchmarks/<benchmark-name>

Funnily enough, the try-catch was introduced because there was a not reproducible error in IE9 ...

@drpicox

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2016

Thanks!

Ok! I'll add the comment now.
Thanks for howto about benchpress, I hope I can try it later.

@drpicox drpicox force-pushed the drpicox:perf-collectDirectives branch 3 times, most recently from d3fd76e to d5351e5 Jun 30, 2016

@drpicox drpicox force-pushed the drpicox:perf-collectDirectives branch from d5351e5 to 3a40bd4 Jun 30, 2016

break;
}

directives.sort(byPriority);
return directives;

function collectCommentDirectives() {

This comment has been minimized.

Copy link
@dcherman

dcherman Jun 30, 2016

Contributor

Rather than extracting the entire case to a function, you may want to consider extracting just the bit that might throw (accessing nodeValue) and putting that function as a sibling to the collectDirectives function.

Reason being is that since collectDirectives is invoked once for every node which is often in the thousands (1558 on a quick sample I took from one app that I work on), we can avoid creating thousands of additional functions that need to be GCed.

This comment has been minimized.

Copy link
@Narretz

Narretz Jun 30, 2016

Contributor

Isn't this something that an interpreter can optimize as it knows that this function is always used the same, only with a different context?

This comment has been minimized.

Copy link
@jbedard

jbedard Jun 30, 2016

Contributor

The interpreter still has to create a new closure and function instance for each and every one though (must ensure two of these != each other etc.).

I'd definitely vote to avoid recreating the same function per node...

This comment has been minimized.

Copy link
@drpicox

drpicox Jun 30, 2016

Author Contributor

Ok,

I have some reasonable doubts about what @dcherman and @jbedard exposes, I believe that current compilers are smart enough, and I would prefer this kind of comments with links to jsperf.

Unfortunately jsperf is offline :/ and in my benchmark with code from real apps shows no conclusive performance difference from one implementation to the other.

So, to let you to test each version I put each version in a separated commit so you can do your own benchmarking.

And about optimisation has this great rule: "don’t optimize before you need to optimize"

This comment has been minimized.

Copy link
@dcherman

dcherman Jun 30, 2016

Contributor

So I'll retract my statement, and you can revert the commit if you'd like.

http://webcache.googleusercontent.com/search?q=cache:9UdL1nWH_xsJ:ariya.ofilabs.com/2012/07/lazy-parsing-in-javascript-engines.html+&cd=1&hl=en&ct=clnk&gl=us

All browsers at least back to IE9 seem to defer parsing of functions until necessary, and memory allocations did not seem to occur in the tests that I've done in Chrome, FF, or IE11.

About pre-mature optimization; the compile and linking phases are some of the hottest areas in Angular, so I think it's more important to consider the implications of any given change here versus tinkering with something like $http where optimization is way less likely to bear measurable fruit. Worst case scenario is you learn something about how browsers work internally as we did here, and that's never a bad thing. In this case, I suggested the change initially because it's semantically identical to the original PR without any additional complexity or significant restructuring, but is more obviously correct to someone that was not aware of engine optimizations (guilty as charged here).

If you want to look into something similar, I've been meaning to make a similar change in $digest to eliminate the deopts there and profile to see if it speeds up the digest cycle at all. I suspect that

refactor($compile): move collectCommentDirectives outside collectDire…
…ctives because a possible performance concern
@petebacondarwin

This comment has been minimized.

Copy link
Member

commented Jul 1, 2016

LGTM

@drpicox

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2016

So, it is ready for merge then.

@petebacondarwin

This comment has been minimized.

Copy link
Member

commented Jul 2, 2016

Yep - will do on Monday if no one gets to it before me

@gkalpak gkalpak closed this in 3aedb1a Jul 4, 2016

@gkalpak

This comment has been minimized.

Copy link
Member

commented Jul 4, 2016

Too low hanging a fruit - sorry, @petebacondarwin 😛

gkalpak added a commit that referenced this pull request Jul 4, 2016

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.