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

Allow assert to be optimized #71

Merged
merged 3 commits into from Aug 4, 2014
Merged

Allow assert to be optimized #71

merged 3 commits into from Aug 4, 2014

Conversation

@geek
Copy link
Member

geek commented Aug 1, 2014

If you run hoek with --trace_opt you can see that the exports.assert isn't optimized because it calls slice on arguments:

disabled optimization for 0x1c4fd5e40f49 <SharedFunctionInfo exports.assert>, reason: Bad value context for arguments value]

After changing this to manually populate msgs from the arguments the function can be optimized by the optimizing compiler.

geek added 2 commits Aug 1, 2014
@@ -413,7 +413,11 @@ exports.assert = function (condition /*, msg1, msg2, msg3 */) {
return;
}

var msgs = Array.prototype.slice.call(arguments, 1);
var msgs = [];
for (var i = 1, il = arguments.length; i < il; ++i) {

This comment has been minimized.

Copy link
@arb

arb Aug 1, 2014

Contributor

I'm not so sure about this change. I have no doubt this is faster and can be optimized, but there is probably a reason no one really does it this way and always uses the Array.prototype.slice.call trick.

Check out this implementation. https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#32-leaking-arguments. That looks like the way to go to me personally if we are trying to squeeze optimizations.

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Aug 2, 2014

Hmm. First, doesn't the early return already void optimization? And even if the function is not optimized, what impact does this have when the condition is true? I assume none. Are we really trying to optimize when the server is about to go boom?!

@hueniverse hueniverse self-assigned this Aug 2, 2014
@geek

This comment has been minimized.

Copy link
Member Author

geek commented Aug 4, 2014

@hueniverse from looking at the deoptimization statements... this is able to be optimized with the early return from what I saw.

It seemed like an easy win since we call assert so frequently.

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Aug 4, 2014

Ok. Can you add a comment documenting why we are not using slice() in this case?

@geek

This comment has been minimized.

Copy link
Member Author

geek commented Aug 4, 2014

@hueniverse added

hueniverse added a commit that referenced this pull request Aug 4, 2014
Allow assert to be optimized
@hueniverse hueniverse merged commit a05d82f into hapijs:master Aug 4, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@hueniverse hueniverse added this to the 2.4.1 milestone Aug 4, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.