1821935 - gfxContext constructor does always an extra heap allocation for mStateStack

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

https://searchfox.org/mozilla-central/rev/2ede53e39955988f98db4369f7ce09614b22104a/gfx/thebes/gfxContext.cpp#55,62
The allocation tends to show up in React-Stockcharts profiles, at least in the ZoomTheChart-sync part.
Could AutoTArray be used in gfxContext?

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Looks like we recently fixed similar bug 1812729 and bug 1812774.

Comment 2

[Jonathan Kew [:jfkthame]]

Yes, I think this would be reasonable. Many of our gfxContexts are short-lived, stack-allocated instances and don't do any Save/Restore operations, so a single-element auto-array would avoid a separate allocation for these.

Actually, on further consideration I think we can do a bit better. We're constantly accessing CurrentState(), which with the existing array implementation (whether the existing nsTArray or an AutoTArray) involves looking up the current array length, and then indexing by mLength-1 to read the last element. We can streamline all these accesses by storing the current AzureState directly in gfxContext, and only pushing it to an array when we do a Save() operation.

This will optimize all the routine access to elements of the state; the one thing that becomes slightly trickier is when we need to iterate over all the clips in the entire state stack, as that'll need to consider both the stack of saved states (if any) and the separately-stored current state.

I'll put up a patch after some local testing.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1821935 - Store current state directly in gfxContext, to avoid a separate allocation in the common case where Save/Restore is not used on the context. r=#gfx-reviewers

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5b0011efcb58
Store current state directly in gfxContext, to avoid a separate allocation in the common case where Save/Restore is not used on the context. r=gfx-reviewers,lsalzman

Comment 5

[Atila Butkovits]

Backed out for causing bustages at gfxContext.h.

Backout link: https://hg.mozilla.org/integration/autoland/rev/7ea8042eaf1d0c5f78ccce2e79e3e13f73526c64

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=689b845c477e76d6f3ef8fa21ef354e86606f75c

Failure log: https://treeherder.mozilla.org/logviewer?job_id=408760067&repo=autoland&lineNumber=5809

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f34761df5a65
Store current state directly in gfxContext, to avoid a separate allocation in the common case where Save/Restore is not used on the context. r=gfx-reviewers,lsalzman

Comment 7

[Norisz Fay [:noriszfay]]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/7ea8042eaf1d

Comment 8

[Cristina Horotan [:chorotan]]

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