1822184 - Consider to use AutoTArray for ContextState::clipsAndTransforms

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, enhancement)

Description

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

https://share.firefox.dev/3JLItJf
https://searchfox.org/mozilla-central/rev/af78418c4b5f2c8721d1a06486cf4cf0b33e1e8d/dom/canvas/CanvasRenderingContext2D.h#946
AppendElement does allocate. Maybe we could avoid that in the most common cases by using an AutoTArray?

The testcase is https://speedometer-preview.netlify.app/interactiverunner React-Stockcharts

Comment 1

[Jonathan Kew [:jfkthame]]

I did a bit of random browsing (various canvas demos/tests, and some Google Docs) with an instrumented build, and found that in the overwhelming majority of instances, we only ever append one element to this array. So yes, we should try to eliminate that allocation.

Rather than an AutoTArray, where we'll end up using LastElement() as the "current" state -- which means every access has to look up the array length and use it as an index -- maybe we should store a ClipState directly in the ContextState, along with an auxiliary (non-auto) array, usually unused, to push any saved states to. The overall footprint of that should be comparable to a 1-element AutoTArray, but with optimized access for the current state.

[edit: After looking over the code, I don't think that's worth doing, as our usage patterns mostly aren't focused on just accessing the "current" (last) element but on iterating over the states. So the simple AutoTArray approach makes most sense here.]

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1822184 - Create an ElementOrArray<T> helper in CanvasRenderingContext2D, and use this to store a single ClipState directly without a separate array allocation. r=#gfx-reviewers

Comment 3

[Jonathan Kew [:jfkthame]]

It doesn't work to simply use AutoTArray here because the canvas context has an array of ContextStates, which requires ContextState to be memmovable.

In theory we could probably resolve that by adding ContextState to the types declared here as needing to use move constructors rather than memmove. But that would end up pretty invasive, as we can't forward-declare the nested ContextState class; we'd have to hoist it out of CanvasRenderingContext2D, and expose it more widely. And no longer being able to memmove the state would come with its own performance impact.

So instead, I propose to use a memmove-able helper to store a single ClipState element directly in the ContextState, and convert to an nsTArray only when more than one element is pushed to the list.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f38530a9bedf
Create an ElementOrArray<T> helper in CanvasRenderingContext2D, and use this to store a single ClipState directly without a separate array allocation. r=gfx-reviewers,lsalzman

Comment 5

[Natalia Csoregi [:nataliaCs]]

Backed out for causing build bustage on CanvasRenderingContext2D.h

• backout: https://hg.mozilla.org/integration/autoland/rev/adc79448e93c6417471115a11ffa3bc6535eb698
• push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=f38530a9bedff6ecc220b7b1a7deeed58b74acea&selectedTaskRun=Mx2m1s7UQbSNLE9SZVPRxw.0
• failure log: https://treeherder.mozilla.org/logviewer?job_id=409265936&repo=autoland&lineNumber=8912

[task 2023-03-17T11:44:54.912Z] 11:44:54     INFO -  In file included from /builds/worker/checkouts/gecko/dom/canvas/CanvasGradient.cpp:7:0,
[task 2023-03-17T11:44:54.912Z] 11:44:54     INFO -                   from Unified_cpp_dom_canvas0.cpp:11:
[task 2023-03-17T11:44:54.913Z] 11:44:54     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/dom/CanvasRenderingContext2D.h: In member function 'T* mozilla::dom::CanvasRenderingContext2D::ElementOrArray<T>::AppendElement(const T&) [with T = mozilla::dom::CanvasRenderingContext2D::ClipState]':
[task 2023-03-17T11:44:54.913Z] 11:44:54    ERROR -  /builds/worker/workspace/obj-build/dist/include/mozilla/dom/CanvasRenderingContext2D.h:147:5: error: control reaches end of non-void function [-Werror=return-type]
[task 2023-03-17T11:44:54.913Z] 11:44:54     INFO -       }
[task 2023-03-17T11:44:54.914Z] 11:44:54     INFO -       ^
[task 2023-03-17T11:44:54.914Z] 11:44:54     INFO -  In file included from Unified_cpp_dom_canvas0.cpp:38:0:
[task 2023-03-17T11:44:54.914Z] 11:44:54     INFO -  /builds/worker/checkouts/gecko/dom/canvas/CanvasRenderingContext2D.cpp: In member function 'mozilla::dom::TextMetrics* mozilla::dom::CanvasRenderingContext2D::DrawOrMeasureText(const nsAString&, float, float, const mozilla::dom::Optional<double>&, mozilla::dom::CanvasRenderingContext2D::TextDrawOperation, mozilla::ErrorResult&)':
[task 2023-03-17T11:44:54.915Z] 11:44:54  WARNING -  /builds/worker/checkouts/gecko/dom/canvas/CanvasRenderingContext2D.cpp:4448:10: warning: 'isRTL' may be used uninitialized in this function [-Wmaybe-uninitialized]
[task 2023-03-17T11:44:54.915Z] 11:44:54     INFO -     aError = nsBidiPresUtils::ProcessText(
[task 2023-03-17T11:44:54.915Z] 11:44:54     INFO -     ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2023-03-17T11:44:54.915Z] 11:44:54     INFO -         textToDraw.get(), textToDraw.Length(),
[task 2023-03-17T11:44:54.916Z] 11:44:54     INFO -         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2023-03-17T11:44:54.916Z] 11:44:54     INFO -         isRTL ? intl::BidiEmbeddingLevel::RTL() : intl::BidiEmbeddingLevel::LTR(),
[task 2023-03-17T11:44:54.916Z] 11:44:54     INFO -         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2023-03-17T11:44:54.916Z] 11:44:54     INFO -         presContext, processor, nsBidiPresUtils::MODE_DRAW, nullptr, 0, nullptr,
[task 2023-03-17T11:44:54.917Z] 11:44:54     INFO -         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2023-03-17T11:44:54.917Z] 11:44:54     INFO -         mBidiEngine);
[task 2023-03-17T11:44:54.917Z] 11:44:54     INFO -         ~~~~~~~~~~~~
[task 2023-03-17T11:44:54.917Z] 11:44:54     INFO -  cc1plus: all warnings being treated as errors

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/96d01e70878e
Create an ElementOrArray<T> helper in CanvasRenderingContext2D, and use this to store a single ClipState directly without a separate array allocation. r=gfx-reviewers,lsalzman

Comment 7

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/96d01e70878e