Large speed ups in iterating over all mappings #308
Conversation
lib/util.js
Outdated
| @@ -89,6 +89,25 @@ function normalize(aPath) { | |||
| var isAbsolute = exports.isAbsolute(path); | |||
|
|
|||
| var parts = path.split(/\/+/); | |||
There was a problem hiding this comment.
nit: the call to path.split() hasn't been removed yet ;)
| const cache = []; | ||
|
|
||
| return function (input) { | ||
| for (var i = 0; i < cache.length; i++) { |
There was a problem hiding this comment.
Is this faster than Array.indexOf?
There was a problem hiding this comment.
We can't use indexOf here because the data in the array is an Object and so will be tested for pointer equality.
We could use Array.prototype.find, but now we're introducing functions and things, and the whole point of this code is to gain whatever control we can over execution, which higher-order-functions can be a bit anathema to.
|
I couldn't load the "files changed" view for some reason, but this is r+ from me, with the comment in the first patch addressed. The thing about |
`String.prototype.split` is slow with regexps. Writing out the C-style string
searching code makes us take .8x the time on the "iterate.already.parsed"
benchmark from the "wasm" branch.
With the old String.prototype.split:
Samples Total (ms) Mean (ms) Standard Deviation (ms)
50 325864.18 6517.28 162.15
With the new custom splitting:
Samples Total (ms) Mean (ms) Standard Deviation (ms)
50 257604.64 5152.09 221.19
We are almost always normalizing the same strings over and over again. Think about when iterating over mappings: we are reconstructing each mapping's source, but the mappings are in sorted order, and so will likely have the same source every time. This makes us take .09x the time that we used to take on the "iterate.already.parsed" benchmark!! Without the LRU cache: Samples Total (ms) Mean (ms) Standard Deviation (ms) 50 257604.64 5152.09 221.19 With the new LRU cache: Samples Total (ms) Mean (ms) Standard Deviation (ms) 50 23301.74 466.03 56.14
fitzgen
force-pushed
the
split-is-slow
branch
from
87604d0 to
83b8bbd
Compare
|
Thanks for review! |
|
The queue for OSX machines in Travis is very long, going to go ahead and merge this now, since Linux is OK and it WFM on OSX locally. |
r? @tromey
This is beneficial for both the extant JS and the incoming wasm in #306.
Order of magnitude speed up iterating over all mappings by throwing a dumb LRU cache in front of the
normalizefunction.Ignore last commit in review -- all mechanical.