1906312 - Use external string cache more for NewStringFromBuffer

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Jan de Mooij [:jandem]]

In NewStringFromBuffer we're using this cache for the inline-Latin1 fast path but not the other cases.

This should fix a regression from bug 1903037 on the "perf_reftest_singletons id-getter" micro-benchmarks that benefit a lot from this cache.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Bug 1906312 - Use external string cache more for NewStringFromBuffer. r?sfink!

Use the cache for NewMaybeExternalString also for the non-inline-Latin1 path
in NewStringFromBuffer.

This fixes a regression from bug 1903037 on the "perf_reftest_singletons id-getter"
micro-benchmarks that benefit a lot from this cache.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/82d75da2b035
Use external string cache more for NewStringFromBuffer. r=sfink

Comment 3

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/82d75da2b035

Comment 4

[Mayank Bansal]

Seems to have improved the numbers somewhat. Pre-regression: 320ms, Post-regression: 750ms, Post-fix: 580ms

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 9411269 [details]
Bug 1906312 - Use external string cache more for NewStringFromBuffer. r?sfink!

Beta/Release Uplift Approval Request

• User impact if declined: Potential performance or memory usage issues.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Patch has been in Nightly for a while and is straight-forward.
• String changes made/needed: N/A
• Is Android affected?: Yes

Comment 6

[Donal Meehan [:dmeehan]]

Comment on attachment 9411269 [details]
Bug 1906312 - Use external string cache more for NewStringFromBuffer. r?sfink!

Approved for 129.0b5

Comment 7

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/a3ef9a7c5aef

Comment 8

[Andrej (:aglavic)]

(In reply to Pulsebot from comment #2)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/82d75da2b035
Use external string cache more for NewStringFromBuffer. r=sfink

Perfherder has detected a talos performance change from push 86f824b9ee1956da8f9753cdeaeaa210c1f90342.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
27%	perf_reftest_singletons id-getter-4.html	windows11-64-shippable-qr	e10s fission stylo webrender	418.54 -> 307.45
26%	perf_reftest_singletons id-getter-7.html	windows11-64-shippable-qr	e10s fission stylo webrender	413.31 -> 307.49
26%	perf_reftest_singletons id-getter-3.html	windows11-64-shippable-qr	e10s fission stylo webrender	413.41 -> 307.75
25%	perf_reftest_singletons id-getter-6.html	windows11-64-shippable-qr	e10s fission stylo webrender	413.21 -> 309.33
25%	perf_reftest_singletons id-getter-5.html	windows11-64-shippable-qr	e10s fission stylo webrender	412.95 -> 309.41
...	...	...	...	...
21%	perf_reftest_singletons id-getter-5.html	macosx1015-64-shippable-qr	e10s fission stylo webrender	812.13 -> 638.34

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 1149

For more information on performance sheriffing please see our FAQ.