240273 - nsRenderingContextGTK::PushState/PopState are malloc-happy

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Each call to PushState allocates a state and a transform matrix; each call to
PopState deletes them.  It looks like someone once tried to semi-solve that
problem with the code inside the USE_GS_POOL ifdefs, but those are not defined
by default and don't go far enough (don't allocate the matrix, eg).

We should really be allocating states and transform matrices out of that pool or
out of an arena or something.  Actually, I'd say using nsFixedSizeAllocator here
would be a nice win for us (using the same allocator with two different bucket
sizes to handle both matrices and states).

The other platforms may want to do this too, but the GTK port is the one I was
profiling.  ;)

To put some perspective on this, for a table row with a few cells of text in it,
50% of the time painting the table row is spent pushing and popping states, most
of it in operator new and delete.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

blizzard, tor, maybe you want to fix this up?

Comment 2

[tor]

Attachment: quick try at nsFixedSizeAllocator

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm.. the trans matrix tests are reversed, and if there is no pool it looks like
matrices are never deleted..

Comment 4

[Boris Zbarsky [:bzbarsky]]

Mental note; I saw this when profiling scrolling on
https://www.seven4sky.com/weblogs/index.php?view=list&show=file&logs=apachesave&file=seven4sky.com-access_log.08-19-2002

Comment 5

[tor]

Attachment: ahem

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 146111 [details] [diff] [review]
ahem

>+  if (gStatePool) {

Nice, pointer test.

>+    void *space = gStatePool->Alloc(sizeof(nsTransform2D));
>+    if (nsnull == mTranMatrix)

Ick, nsnull == instead of !.  Drive-by nit, I know, but can we shed this
sophomoric MS coding style for equality (and inequality) tests?

/be

Comment 7

[Boris Zbarsky [:bzbarsky]]

I'll give that patch a profile tonight to see how it affects things.

Comment 8

[tor]

Attachment: now with less nsnull

FWIW, the initial size of the pool is just a guess.

Comment 9

[Boris Zbarsky [:bzbarsky]]

So profiling scrolling on the url in comment 4, I see the following results:

Without this patch:
233874 hits under nsTableRowGroupFrame::PaintChildren.  Of these:
  72598 under nsRenderingContextGTK::PopState
  65103 under nsRenderingContextGTK::PushState
  40494 under nsTableRowFrame::Paint

With this patch:
237939 hits under nsTableRowGroupFrame::PaintChildren.  Of these:
  65294 under nsTableRowFrame::Paint
  44878 under nsRenderingContextGTK::PopState
  33220 under nsRenderingContextGTK::PushState

The fact that the total number of hits is pretty much the same makes sense (this
is a realtime profile, and I ran it for the same amount of wall-clock time). 
Note that PopState and PushState are both 1.5-2 times faster than without this
patch, leaving more time for actual painting.

Comment 10

[Roland Mainz]

T Rowley (IBM) wrote:
> Created an attachment (id=146131)
> now with less nsnull

Do you have time to do the same for the Xlib toolkit, please ? If "not" please
reassign the bug to me that I can work on that next week...

Comment 11

[tor]

Checked in.