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

Strict Mode Actually Worth It #53

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kemitchell
Copy link

On less than the latest-and-greatest, V8-TurboFan Chrome, performance tracing runs of apps bundling hyperx produce the dismaying notice "Not optimized: Bad value context for arguments value" for functions returned by hyperx. Best I can tell, V8-Crankshaft bails on optimizing the functions because of the way they use arguments, and the fact that vanilla JS treats arguments[0] as an alias for x in function (x) { ... }. This causes problems when references from arguments get passed on to other functions.

I've spent a little time scratching my head, trying various approaches, like allocating a new Array and copying references over from arguments, to avoid the issue. Someone else might be able to do that more reliably. And anyone who cares to try, or comes upon this later, might like to know that Node.js has a nifty --trace_deopt flag to print V8's warnings.

But it occurred to me that it's far easier to just opt into Strict Mode, where argument aliasing is not a thing. I've confirmed that 'use strict' coaxes V8-Crankshaft into optimizing the functions, which is a pretty big win, considering when and how often those functions run. And that the tests all pass.

That's a lot of background for a one-line patch adding the Strict Mode statement. But I wouldn't usually send such a thing---I don't use Strict Mode much in my own code, and have seen a fair number of irksome "support --use_strict" GitHub issues. Here I get the feeling it might make sense.

Thanks,

K

@ghost
Copy link

ghost commented Jul 17, 2017

I ran the benchmark code in bench/ but I didn't see a difference in performance time between using strict or not:

$ # before
$ node --version
v6.3.1
$ for x in {1..3}; do node raw.js; done
0.2277 'ms'
0.2247 'ms'
0.2377 'ms'
$ for x in {1..3}; do node loop.js; done
0.2645 'ms'
0.2642 'ms'
0.2658 'ms'

$ # after
$ node --version
v6.3.1
$ git checkout strict
$ for x in {1..3}; do node raw.js; done
0.2356 'ms'
0.2361 'ms'
0.2398 'ms'
$ for x in {1..3}; do node loop.js; done
0.2676 'ms'
0.2703 'ms'
0.2587 'ms'

---

$ # before
$ nave use 8.1.4
$ git checkout master
$ for x in {1..3}; do node raw.js; done
0.1591 'ms'
0.1318 'ms'
0.1544 'ms'
$ for x in {1..3}; do node loop.js; done
0.2008 'ms'
0.2024 'ms'
0.2013 'ms'

$ # after
$ node --version
v8.1.4
$ git checkout strict
$ for x in {1..3}; do node raw.js; done
0.1554 'ms'
0.1505 'ms'
0.1545 'ms'
$ for x in {1..3}; do node loop.js; done
0.2048 'ms'
0.2033 'ms'
0.2051 'ms'

---

$ # before
$ # chrome Version 58.0.3018.3 (Official Build) dev (64-bit)
$ git checkout master
$ budo raw.js
0.1551 "ms"
0.1683 "ms"
0.1602 "ms"
$ budo loop.js
0.2113 "ms"
0.2064 "ms"
0.2084 "ms"

$ # after
$ # chrome Version 58.0.3018.3 (Official Build) dev (64-bit)
$ git checkout strict
$ budo raw.js
0.1593 "ms"
0.1643 "ms"
0.1588 "ms"
$ budo loop.js
0.2109 "ms"
0.2102 "ms"
0.2155 "ms"

@kemitchell
Copy link
Author

Hmm. Will investigate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant