353882 - performance enhancements to nsCSSFrameConstructor::ConstructFrameByDisplayType

Status: RESOLVED INVALID

(Core :: Layout: Block and Inline, defect)

Description

[Robert Sayre]

When profiling Tp2, Shark says we spend about 2% of our time inside this method in an optimized debug Cocoa build, in one call stack (from CNavDTD::CloseContainer). The patching coming up takes it down to 1.1%. There are other callers that account for about the same amount of samples, but I haven't examined them as closely yet.

Comment 1

[Robert Sayre]

Attachment: make a temp variable, inline a few methods

Comment 2

[Robert Sayre]

Attachment: make a temp variable, inline a few methods

Comment 3

[Boris Zbarsky [:bzbarsky]]

So wait.  Those methods should already be inline, since the impl is right there... As in, adding the |inline| keyword really shouldn't affect anything.  Does it?  If it does, we may want to add it elsewhere in our tree; we use the "implemented in the class decl means it's inline" pattern a _lot_.

I'd call the temp variable |display| or maybe |displayValue|, not |tempDisplay|.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 239725 [details] [diff] [review]
make a temp variable, inline a few methods

The |inline| should already be implied.

And please don't name variables temp* when there's nothing particularly temporary about them.  |display| or |displayStyle| should be fine.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And I've seen many times in disassembly (looking at talkback) that methods written out in the class definition are inlined.  I'd be quite surprised if adding the keywords made a difference.  (Quick ways to tell would be to 'make nsCSSFrameConstructor.s' and look at the assembly.  (Remember to delete *.s when you're done looking; otherwise it will mess up your tree.)  You could also check if the size of libgklayout.so changed with the nsStyleStruct.h patch.)

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

For example, in my x86_64 opt build, this code:

  if (aDisplay->IsBlockLevel() &&
      aDisplay->mDisplay != NS_STYLE_DISPLAY_TABLE &&
      aDisplay->IsScrollableOverflow() &&
      !propagatedScrollToViewport) {

compiles to this:

        movzbl  28(%r12), %edx
        cmpb    $4, %dl            NS_STYLE_DISPLAY_LIST_ITEM is 4
        sete    %cl
        cmpb    $1, %dl            NS_STYLE_DISPLAY_BLOCK is 1
        sete    %al
        orb     %cl, %al
        movl    %eax, %edi
        je      .L3733
        cmpb    $8, %dl            NS_STYLE_DISPLAY_TABLE is 8 (half-optimized)
        je      .L3733
        movzbl  36(%r12), %eax
        testb   %al, %al           NS_STYLE_OVERFLOW_VISIBLE is 0
        je      .L3733
        cmpb    $4, %al            NS_STYLE_OVERFLOW_CLIP is 4
        .p2align 4,,3
        je      .L3733
        testl   %esi, %esi         %esi holds propagatedScrollToViewport
        .p2align 4,,3
        jne     .L3733

Comment 7

[Robert Sayre]

(In reply to comment #5)
> And I've seen many times in disassembly (looking at talkback) that methods
> written out in the class definition are inlined.  I'd be quite surprised if
> adding the keywords made a difference.

Yeah, it looks like 'inline' is not doing anything. In my tests, doing it seemed to make a difference, but it may have just been chance. The addition of the local variable definitely made a much bigger difference. I will retest before submitting a new patch.

Comment 8

[Robert Sayre]

http://people.mozilla.com/~sayrer/2006/09/23/tp2/ 

is a PlotKit graph of the means from the Tp2 tests I ran. 

The trunk code is *a lot* noisier than the patched code. There was nothing I did differently, and I ran the test a bunch of times. The minimum sample from the trunk code was often within the standard deviation of the patched code. 

The standard deviations on tests where there was no meaningful difference was pretty similar. On tests where the code diverged, the trunk's stddev was an order of magnitude higher.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 239725 [details] [diff] [review]
make a temp variable, inline a few methods

r- pending investigation

Comment 10

[Robert Sayre]

measurement error