1371979 - Avoid some memory allocation / deallocation in DrawTargetTiled::PushClipRect / PopClip

Status: RESOLVED FIXED

(Core :: Graphics, enhancement)

Description

[Markus Stange [:mstange]]

mClippedOutTilesStack can use some inline storage, and its elements can have a fixed size.

The patch in this bug reduces paint times on attachment 8872204 [details] from 44ms to 36ms for me.

Comment 1

[Markus Stange [:mstange]]

Attachment: Bug 1371979 - Reduce memory allocation / deallocation in DrawTargetTiled::PushClip(Rect) / PopClip.

Review commit: https://reviewboard.mozilla.org/r/147798/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147798/

Comment 2

[Markus Stange [:mstange]]

Profile before: https://perfht.ml/2rO0XR7
Profile after: https://perfht.ml/2rNMzIy

Comment 3

[Bas Schouten (:bas.schouten)]

Comment on attachment 8876458 [details]
Bug 1371979 - Reduce memory allocation / deallocation in DrawTargetTiled::PushClip(Rect) / PopClip.

https://reviewboard.mozilla.org/r/147798/#review152320

I'm not completely against this, but some comments:

The right way to do this using the STL is to use a custom allocator and doing something like this: https://chromium.googlesource.com/chromium/chromium/+/master/base/stack_container.h

But I suppose it doesn't matter too much, I expect vector<> to perform better than Vector<> in general, but in this case it's unlikely to matter.

It is important to note this is a significant size increase, in the case of 50 tiles and a 5 level deep clip (not that exceptional), we're talking 2KB, for pathelogical cases where we create lots of small DrawTargets that could be significant, although maybe that big an amount of DrawTargets has other issues on its own. If we're just trying to minimize allocation load it may be better to stick with a std::vector<uint32_t> and do a .reserve on it to some smaller, but reasonable number. Although your method may have a small performance benefit.

::: gfx/2d/DrawTargetTiled.cpp:119
(Diff revision 1)
>  TILED_COMMAND3(Mask, const Pattern&, const Pattern&, const DrawOptions&)
>  
>  void
>  DrawTargetTiled::PushClip(const Path* aPath)
>  {
> -  mClippedOutTilesStack.push_back(std::vector<uint32_t>());
> +  if (!mClippedOutTilesStack.append(std::vector<bool>(mTiles.size()))) {

It's so annoying that Vector doesn't just support the STL push_back syntax..

Comment 4

[Markus Stange [:mstange]]

(In reply to Bas Schouten (:bas.schouten) from comment #3)
> in the case of
> 50 tiles and a 5 level deep clip (not that exceptional), we're talking 2KB,
> for pathelogical cases where we create lots of small DrawTargets that could
> be significant

Our DrawTargets are at least 256x256 pixels big. So each one of those already takes up 256 * 256 * 4B ≈ 262KB of space, and 50 of them take up 13MB. I'd be surprised if the additional 2KB make a difference in that case.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6a16797cc2b5
Reduce memory allocation / deallocation in DrawTargetTiled::PushClip(Rect) / PopClip. r=bas

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6a16797cc2b5