Fix RangeError, and make it a teeny bit faster #8
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There's a limit to how many arguments a JavaScript VM will accept for a single function call, and piping large blocks of the game image through String.fromCharCode.apply appears to violate that limit.
Setting the length of an array is surprisingly expensive -- instead, just return a new literal array. Also use native array slicing when possible.
erkyrath
added a commit
that referenced
this pull request
May 29, 2015
Fix RangeError, and make it a teeny bit faster
|
Note to observers: this bug manifests itself when playing a game with large images loaded from a gblorb.js file (not the fast-loading path). Counterfeit Monkey shows it. Imagetest and Sensory Jam do not, because all their image files are tiny. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The RangeError also happens with the 2.1.0 release; I think this commit should apply cleanly there, though.
Times are all load time of Counterfeit Monkey (as given in the console) averaged over three attempts, in current Chromium and current Firefox.
2.1.0 release, after fixing RangeError:
Chromium: 33.758s
Firefox: 16.190s
noeval, after fixing RangeError:Chromium: 18.751s
Firefox: 22.590s
noeval, after speeding upfetch_search_key:Chromium: 16.559s
Firefox: 21.783s
So, seems to match your observations — Chrome is significantly faster, Firefox is significantly slower. Chrome started out doing really badly, though.
I'm not quite sure where to go from here; the VMs are just optimizing differently, and now the bulk of the time is in a handful of functions so it's difficult to see what low-hanging fruit remains. Chromium blames 28% of the time on
execute_loopand 18% onpop_callstub; Firefox blames 42% (!) onexecute_loopand 19% on all the dynamically-generated functions in aggregate.Just for fun, my original branch, which is kind of a mess:
Chromium: 11.934s
Firefox: 16.242s
The only significant difference I can think of is that I made a VM "class", with all the functions on its prototype, and then passed a single instance to generated functions as a regular argument. It's possible that both JavaScript VMs are just much better at dealing with this pattern, since it's how new types are generally implemented.