1903981 - Send the scene back to the scene builder thread instead of dropping it in the renderer

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement)

Description

[Nicolas Silva [:nical]]

The goal is to avoid deallocating the scene in a different thread from the one where it was allocated, to avoid lock contention in jemalloc.

Comment 1

[Nicolas Silva [:nical]]

Attachment: Bug 1903981 - Do scene-related memory deallocations on the scene builder thread. r=gw

The scene is sent back to the scene builder thread where the SceneRecycler will reuse some allocations and drop the rest. Deallocating the memory on the scene builder thread where it was allocated removes lock contention in jemalloc and as a bonus gives an opportunity to recycle some of it. This patch picks only the lowest hanging fruits when it comes to recycling allocations from the scene and the scene builder.

Comment 2

[Nicolas Silva [:nical]]

Attachment: WIP: Bug 1903981 - Do scene-related memory deallocation on the scene builder thread. @gw

The scene is sent back to the scene builder thread where the SceneRecycler will reuse some allocations and drop the rest. Deallocating the memory on the scene builder thread where it was allocated removes lock contention in jemalloc and as a bonus gives an opportunity to recycle some of it. This patch picks only the lowest hanging fruits when it comes to recycling allocations from the scene and the scene builder.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/04d0c38e624d
Do scene-related memory deallocations on the scene builder thread. r=gw

Comment 4

[Serban Stanca [:SerbanS]]

Backed out for causing multiple build bustages.

• Backout link
• Push with failures
• Failure Log

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1b1f41f96fb
Do scene-related memory deallocations on the scene builder thread. r=gw

Comment 6

[tszentpeteri]

https://hg.mozilla.org/mozilla-central/rev/f1b1f41f96fb

Comment 7

[Mayank Bansal]

This appears to have improved displaylist mutate test by 13%

Comment 8

[Alex Finder]

(In reply to Serban Stanca [:SerbanS] from comment #4)

Backed out for causing multiple build bustages.

• Backout link
• Push with failures
• Failure Log

Hi Nicolas! It looks like the following push, which contains the backout mentioned in the previous comment, caused the following performance changes

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
3%	motionmark-htmlsuite-ramp	linux1804-64-shippable-qr	fission webrender	490.30 -> 474.32	Before/After

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
8%	youtube ContentfulSpeedIndex	android-hw-a51-11-0-aarch64-shippable-qr	warm webrender	532.22 -> 490.08	Before/After

Can you help us confirm if it was this backout specifically that caused the performance changes ? Or do you think it's one of the other backouts in this push that caused them ?

Comment 9

[Andrej (:aglavic)]

Removed for typo

Comment 10

[Andrej (:aglavic)]

(In reply to Pulsebot from comment #5)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1b1f41f96fb
Do scene-related memory deallocations on the scene builder thread. r=gw

Perfherder has detected a talos performance change from push f1b1f41f96fba444f4d16acd161b069f20b8ac67.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
14%	displaylist_mutate	windows11-64-shippable-qr	e10s fission stylo webrender	1,633.67 -> 1,406.42
12%	displaylist_mutate	windows11-64-shippable-qr	e10s fission stylo webrender-sw	1,577.11 -> 1,385.33
11%	displaylist_mutate	linux1804-64-shippable-qr	e10s fission stylo webrender	2,902.26 -> 2,575.88
11%	displaylist_mutate	linux1804-64-shippable-qr	e10s fission stylo webrender-sw	2,737.75 -> 2,442.76
10%	displaylist_mutate	linux1804-64-shippable-qr	e10s fission stylo webrender	2,892.14 -> 2,597.74
...	...	...	...	...
3%	tscrollx	linux1804-64-shippable-qr	e10s fission stylo webrender-sw	1.00 -> 0.97

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 1441

For more information on performance sheriffing please see our FAQ.

Comment 11

[Nicolas Silva [:nical]]

Can you help us confirm if it was this backout specifically that caused the performance changes

Yes indeed!

Comment 12

[Andra Esanu (needinfo me)]

(In reply to Pulsebot from comment #5)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1b1f41f96fb
Do scene-related memory deallocations on the scene builder thread. r=gw

Perfherder has detected a browsertime performance change from push f1b1f41f96fba444f4d16acd161b069f20b8ac67.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
4%	motionmark-htmlsuite-1-3	windows11-64-shippable-qr	fission webrender	925.25 -> 966.12	Before/After
4%	motionmark-htmlsuite-ramp	linux1804-64-shippable-qr	fission webrender	482.60 -> 501.13	Before/After
4%	motionmark-htmlsuite-1-3	linux1804-64-shippable-qr	fission webrender	527.86 -> 547.91	Before/After
3%	motionmark-htmlsuite-ramp	windows11-64-shippable-qr	fission webrender	884.15 -> 912.46	Before/After
2%	motionmark-htmlsuite-1-3	linux1804-64-shippable-qr	fission webrender	538.17 -> 546.77	Before/After

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 1479

For more information on performance sheriffing please see our FAQ.

Comment 13

[Alex Finder]

(In reply to Serban Stanca [:SerbanS] from comment #4)

Backed out for causing multiple build bustages.

• Backout link
• Push with failures
• Failure Log

Perfherder has detected a browsertime performance change from push ca70a0bf2a80bd3d1a9a7af2e55838208f08747c.

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
4%	motionmark-htmlsuite-ramp	linux1804-64-shippable-qr	fission webrender	491.94 -> 470.49	Before/After

As author of one of the patches included in that push, we need your help to address this regression.
Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests. Please follow our guide to handling regression bugs and let us know your plans within 3 business days, or the patch(es) may be backed out in accordance with our regression policy.

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 1448

For more information on performance sheriffing please see our FAQ.

Comment 14

[Alex Finder]

(In reply to Nicolas Silva [:nical] from comment #11)

Can you help us confirm if it was this backout specifically that caused the performance changes

Yes indeed!

Thanks for the reply Nicolas, I linked the alert summary to this patch.