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

replace string slices with startsWith comparison #9

Merged
merged 2 commits into from
Oct 3, 2020

Conversation

JoostK
Copy link
Contributor

@JoostK JoostK commented Oct 2, 2020

This commit replaces string slice operations in the lexer with startsWith operations, using an offset position to start the search from. This avoids intermediate string slices so should be easier on the allocator and garbage collector.

I am seeing a performance improvement of -14% on average with these changes, as measured on the following system:

CPU: AMD Ryzen 3700X @ 3.6GHz (turbo'ing disabled for more stable results)
Mem: 64GB
OS: macOS 10.15.2 (Hackintosh)
Node.js: 14.13.0
Samples: 1000 (increased from default 25)

JS Run master PR
test/samples/angular.js (1410 KiB) 11.024ms 9.996ms
test/samples/angular.min.js (303 KiB) 4ms 3.012ms
test/samples/d3.js (553 KiB) 6ms 5.007ms
test/samples/d3.min.js (250 KiB) 3.004ms 3.001ms
test/samples/magic-string.js (34 KiB) 0.163ms 0.019ms
test/samples/magic-string.min.js (20 KiB) 0.002ms 0.003ms
test/samples/rollup.js (698 KiB) 7.025ms 6.003ms
test/samples/rollup.min.js (367 KiB) 5.001ms 4ms
test/samples/*.js (3635 KiB) 36.03ms 30.954ms

The WASM results are not affected, obviously:

WASM Run master PR
test/samples/angular.js (1410 KiB) 8.985ms 9.082ms
test/samples/angular.min.js (303 KiB) 3.001ms 3ms
test/samples/d3.js (553 KiB) 4.977ms 4.996ms
test/samples/d3.min.js (250 KiB) 2.004ms 2ms
test/samples/magic-string.js (34 KiB) 0ms 0ms
test/samples/magic-string.min.js (20 KiB) 0ms 0ms
test/samples/rollup.js (698 KiB) 6ms 6ms
test/samples/rollup.min.js (367 KiB) 3.023ms 3.04ms
test/samples/*.js (3635 KiB) 28.764ms 28.63ms

Raw output:

master (4d56d33)
--- JS Build ---
Module load time
> 9ms
Cold Run, All Samples
test/samples/*.js (3635 KiB)
> 301ms

Warm Runs (average of 1000 runs)
test/samples/angular.js (1410 KiB)
> 11.024ms
test/samples/angular.min.js (303 KiB)
> 4ms
test/samples/d3.js (553 KiB)
> 6ms
test/samples/d3.min.js (250 KiB)
> 3.004ms
test/samples/magic-string.js (34 KiB)
> 0.163ms
test/samples/magic-string.min.js (20 KiB)
> 0.002ms
test/samples/rollup.js (698 KiB)
> 7.025ms
test/samples/rollup.min.js (367 KiB)
> 5.001ms

Warm Runs, All Samples (average of 1000 runs)
test/samples/*.js (3635 KiB)
> 36.03ms
--- Wasm Build ---
Module load time
> 10ms
Cold Run, All Samples
test/samples/*.js (3635 KiB)
> 33ms

Warm Runs (average of 1000 runs)
test/samples/angular.js (1410 KiB)
> 8.985ms
test/samples/angular.min.js (303 KiB)
> 3.001ms
test/samples/d3.js (553 KiB)
> 4.977ms
test/samples/d3.min.js (250 KiB)
> 2.004ms
test/samples/magic-string.js (34 KiB)
> 0ms
test/samples/magic-string.min.js (20 KiB)
> 0ms
test/samples/rollup.js (698 KiB)
> 6ms
test/samples/rollup.min.js (367 KiB)
> 3.023ms

Warm Runs, All Samples (average of 1000 runs)
test/samples/*.js (3635 KiB)
> 28.764ms
PR
--- JS Build ---
Module load time
> 9ms
Cold Run, All Samples
test/samples/*.js (3635 KiB)
> 283ms

Warm Runs (average of 1000 runs)
test/samples/angular.js (1410 KiB)
> 9.996ms
test/samples/angular.min.js (303 KiB)
> 3.012ms
test/samples/d3.js (553 KiB)
> 5.007ms
test/samples/d3.min.js (250 KiB)
> 3.001ms
test/samples/magic-string.js (34 KiB)
> 0.019ms
test/samples/magic-string.min.js (20 KiB)
> 0.003ms
test/samples/rollup.js (698 KiB)
> 6.003ms
test/samples/rollup.min.js (367 KiB)
> 4ms

Warm Runs, All Samples (average of 1000 runs)
test/samples/*.js (3635 KiB)
> 30.954ms
--- Wasm Build ---
Module load time
> 7ms
Cold Run, All Samples
test/samples/*.js (3635 KiB)
> 33ms

Warm Runs (average of 1000 runs)
test/samples/angular.js (1410 KiB)
> 9.082ms
test/samples/angular.min.js (303 KiB)
> 3ms
test/samples/d3.js (553 KiB)
> 4.996ms
test/samples/d3.min.js (250 KiB)
> 2ms
test/samples/magic-string.js (34 KiB)
> 0ms
test/samples/magic-string.min.js (20 KiB)
> 0ms
test/samples/rollup.js (698 KiB)
> 6ms
test/samples/rollup.min.js (367 KiB)
> 3.04ms

Warm Runs, All Samples (average of 1000 runs)
test/samples/*.js (3635 KiB)
> 28.63ms

I am curious to see results on other hardware, as the measurements in the readme show that JS is roughly 47% slower than WASM, but for me it's only 29% on master; this is reduced to just 11% with these changes.

@guybedford
Copy link
Collaborator

I must admit I am somewhat shocked V8 doesn't just do this optimization internally, but the PR looks great thank you. I will update the benchmarks now.

lexer.js Outdated Show resolved Hide resolved
lexer.js Outdated Show resolved Hide resolved
@guybedford guybedford merged commit c4bbbbc into nodejs:master Oct 3, 2020
@guybedford
Copy link
Collaborator

Yeah I'm also seeing a ~20% improvement here, benchmarks updated in 9a551db.

Note that the huge benefit of Wasm is not the warm speed but the cold speed because the compilation doesn't need to be done JIT which saves ~ 250ms of compile time V8 otherwise has to do.

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.

2 participants