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

More tweaks to reduce call stack size #1277

Merged
merged 1 commit into from Aug 31, 2022

Conversation

lahma
Copy link
Collaborator

@lahma lahma commented Aug 31, 2022

Checking some hot paths and ensuring that the stack doesn't contain unnecessary items. The string-validate-input is pretty insane, though the tests still pass 🤷🏻‍♂️

/cc @KurtGokhan @CreepGin

Esprima.Benchmark.SunSpiderBenchmark

Diff Method FileName Mean Error Gen 0 Gen 1 Gen 2 Allocated
Old Run 3d-cube 217.72 ms 0.453 ms 3000.0000 333.3333 - 51,125 KB
New 206.21 ms (-5%) 0.372 ms 3000.0000 (0%) 333.3333 (0%) - 51,125 KB (0%)
Old Run 3d-morph 160.62 ms 0.226 ms 2000.0000 1000.0000 - 49,744 KB
New 158.71 ms (-1%) 0.510 ms 3000.0000 (+50%) 1250.0000 (+25%) 500.0000 49,745 KB (0%)
Old Run 3d-raytrace 173.30 ms 1.197 ms 5000.0000 1000.0000 - 93,390 KB
New 172.14 ms (-1%) 2.352 ms 5000.0000 (0%) 1000.0000 (0%) - 93,390 KB (0%)
Old Run access-binary-trees 86.68 ms 0.349 ms 4000.0000 1000.0000 - 78,472 KB
New 81.73 ms (-6%) 0.177 ms 4000.0000 (0%) 1000.0000 (0%) - 78,472 KB (0%)
Old Run access-fannkuch 469.09 ms 1.075 ms - - - 136 KB
New 455.82 ms (-3%) 2.175 ms - - - 136 KB (0%)
Old Run access-nbody 197.97 ms 0.457 ms 3666.6667 - - 63,853 KB
New 191.97 ms (-3%) 0.484 ms 3666.6667 (0%) 333.3333 - 63,850 KB (0%)
Old Run access-nsieve 157.01 ms 0.295 ms 1000.0000 - - 21,518 KB
New 164.04 ms (+4%) 0.170 ms 1000.0000 (0%) - - 21,518 KB (0%)
Old Run bitop(...)-byte [24] 166.13 ms 0.398 ms 4000.0000 - - 68,950 KB
New 163.70 ms (-1%) 0.509 ms 4000.0000 (0%) - - 68,950 KB (0%)
Old Run bitops-bits-in-byte 262.53 ms 0.422 ms 2000.0000 - - 45,446 KB
New 244.74 ms (-7%) 0.389 ms 2000.0000 (0%) - - 45,446 KB (0%)
Old Run bitops-bitwise-and 178.28 ms 0.559 ms 5000.0000 - - 93,439 KB
New 168.51 ms (-5%) 1.003 ms 5000.0000 (0%) - - 93,439 KB (0%)
Old Run bitops-nsieve-bits 212.82 ms 0.730 ms 3000.0000 1000.0000 - 54,856 KB
New 207.57 ms (-2%) 1.581 ms 3000.0000 (0%) 1000.0000 (0%) - 54,856 KB (0%)
Old Run contr(...)rsive [21] 122.64 ms 0.226 ms 7400.0000 1600.0000 - 124,119 KB
New 116.13 ms (-5%) 0.287 ms 7400.0000 (0%) 1600.0000 (0%) - 124,119 KB (0%)
Old Run crypto-aes 138.35 ms 0.317 ms - - - 15,601 KB
New 135.67 ms (-2%) 0.378 ms - - - 15,590 KB (0%)
Old Run crypto-md5 108.69 ms 0.273 ms 5000.0000 1000.0000 - 95,414 KB
New 108.42 ms (0%) 0.179 ms 5000.0000 (0%) 1000.0000 (0%) - 95,414 KB (0%)
Old Run crypto-sha1 111.83 ms 0.311 ms 5000.0000 1000.0000 - 82,074 KB
New 107.91 ms (-4%) 0.302 ms 5000.0000 (0%) 1000.0000 (0%) - 82,074 KB (0%)
Old Run date-format-tofte 109.06 ms 0.242 ms 2000.0000 - - 48,549 KB
New 107.01 ms (-2%) 0.285 ms 2000.0000 (0%) - - 48,549 KB (0%)
Old Run date-format-xparb 58.78 ms 0.170 ms 1777.7778 666.6667 - 29,414 KB
New 59.77 ms (+2%) 0.073 ms 1777.7778 (0%) 666.6667 (0%) - 29,414 KB (0%)
Old Run math-cordic 349.63 ms 1.410 ms 7000.0000 - - 123,973 KB
New 337.03 ms (-4%) 1.368 ms 7000.0000 (0%) - - 123,973 KB (0%)
Old Run math-partial-sums 138.54 ms 0.279 ms 4000.0000 - - 68,947 KB
New 134.10 ms (-3%) 0.537 ms 4000.0000 (0%) - - 68,947 KB (0%)
Old Run math-spectral-norm 136.91 ms 0.462 ms 4000.0000 - - 67,176 KB
New 135.20 ms (-1%) 0.534 ms 4000.0000 (0%) - - 67,176 KB (0%)
Old Run regexp-dna 108.58 ms 0.399 ms 800.0000 800.0000 800.0000 17,672 KB
New 108.34 ms (0%) 0.383 ms 1000.0000 (+25%) 1000.0000 (+25%) 1000.0000 (+25%) 17,674 KB (0%)
Old Run string-base64 104.31 ms 0.330 ms - - - 12,525 KB
New 103.66 ms (-1%) 0.365 ms - - - 12,523 KB (0%)
Old Run string-fasta 204.44 ms 0.353 ms 10000.0000 333.3333 - 167,275 KB
New 197.59 ms (-3%) 0.456 ms 10000.0000 (0%) 333.3333 (0%) - 167,275 KB (0%)
Old Run string-tagcloud 75.07 ms 0.790 ms 3142.8571 1285.7143 571.4286 48,439 KB
New 73.81 ms (-2%) 1.079 ms 3142.8571 (0%) 1285.7143 (0%) 571.4286 (0%) 48,438 KB (0%)
Old Run string-unpack-code 75.11 ms 0.249 ms 5000.0000 571.4286 - 84,573 KB
New 72.20 ms (-4%) 0.397 ms 5000.0000 (0%) 571.4286 (0%) - 84,573 KB (0%)
Old Run strin(...)input [21] 1,271.89 ms 25.280 ms 696000.0000 692000.0000 691000.0000 6,352,002 KB
New 84.62 ms (-93%) 0.186 ms 2000.0000 (-100%) 1000.0000 (-100%) - 44,618 KB (-99%)

@lahma lahma merged commit 389629c into sebastienros:main Aug 31, 2022
@lahma lahma deleted the more-call-stack-tweaks branch August 31, 2022 17:28
@sebastienros
Copy link
Owner

What's up with the strin(...)input [21] benchmark?

@lahma
Copy link
Collaborator Author

lahma commented Aug 31, 2022

What's up with the strin(...)input [21] benchmark?

I'll try to run the numbers at some point. This was a bit unexpected..

@lahma
Copy link
Collaborator Author

lahma commented Sep 1, 2022

What's up with the strin(...)input [21] benchmark?

OK it's a change in behavior:

https://github.com/sebastienros/jint/pull/1277/files#diff-e715e3b319920d94963426777af53d921e568d5a187aa805a835d1ef4e6063dfL59

The function result is no longer eagerly cloned, in this case string builder (ConcatenatedString) isn't being materialized. I need to think if there is a case where it could be a problem, on assignment we usually want to ensure that it's properly materialized,.

@sebastienros
Copy link
Owner

Another point of view is that since it had such a big impact there, maybe there are other places with the same code that could be optimized the same way, as long as it's also valid.

@lahma
Copy link
Collaborator Author

lahma commented Sep 1, 2022

So far I haven't found a reason this wouldn't generally be safe for string in this case. ConcatenatedString is only one currently using the Clone, I was afraid of ArgumentsInstance, which is pooled and can cause trouble (thanks test suite!). One thing to keep an eye on for sure.

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.

None yet

2 participants