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

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
Closed

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

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3a40bd4
perf($compile): wrap try/catch of collect comment directives into a f…
drpicox Jun 30, 2016
6860801
refactor($compile): move collectCommentDirectives outside collectDire…
drpicox Jun 30, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 19 additions & 13 deletions src/ng/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2052,24 +2052,30 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
addTextInterpolateDirective(directives, node.nodeValue);
break;
case NODE_TYPE_COMMENT: /* Comment */
try {
match = COMMENT_DIRECTIVE_REGEXP.exec(node.nodeValue);
if (match) {
nName = directiveNormalize(match[1]);
if (addDirective(directives, nName, 'M', maxPriority, ignoreDirective)) {
attrs[nName] = trim(match[2]);
}
}
} catch (e) {
// turns out that under some circumstances IE9 throws errors when one attempts to read
// comment's node value.
// Just ignore it and continue. (Can't seem to reproduce in test case.)
}
collectCommentDirectives();
break;
}

directives.sort(byPriority);
return directives;

function collectCommentDirectives() {
Copy link
Contributor

@dcherman dcherman Jun 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@drpicox drpicox Jun 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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"

Copy link
Contributor

Choose a reason for hiding this comment

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

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

// function created because of performance, try/catch disables
// the optimization of the whole function #14848
try {
match = COMMENT_DIRECTIVE_REGEXP.exec(node.nodeValue);
if (match) {
nName = directiveNormalize(match[1]);
if (addDirective(directives, nName, 'M', maxPriority, ignoreDirective)) {
attrs[nName] = trim(match[2]);
}
}
} catch (e) {
// turns out that under some circumstances IE9 throws errors when one attempts to read
// comment's node value.
// Just ignore it and continue. (Can't seem to reproduce in test case.)
}
}
}

/**
Expand Down