performance enhancements to nsCSSFrameConstructor::ConstructFrameByDisplayType

RESOLVED INVALID

Status

()

Core
Layout: Block and Inline
RESOLVED INVALID
12 years ago
11 years ago

People

(Reporter: Robert Sayre, Assigned: Robert Sayre)

Tracking

({perf})

Trunk
PowerPC
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

12 years ago
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.
(Assignee)

Comment 1

12 years ago
Created attachment 239724 [details] [diff] [review]
make a temp variable, inline a few methods
Assignee: nobody → sayrer
Status: NEW → ASSIGNED
(Assignee)

Comment 2

12 years ago
Created attachment 239725 [details] [diff] [review]
make a temp variable, inline a few methods
Attachment #239724 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Attachment #239725 - Flags: review?(bzbarsky)

Updated

12 years ago
Keywords: perf
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 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.
Attachment #239725 - Flags: review-
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.)
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
(Assignee)

Comment 7

12 years ago
(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.
(Assignee)

Comment 8

12 years ago
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 on attachment 239725 [details] [diff] [review]
make a temp variable, inline a few methods

r- pending investigation
Attachment #239725 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 10

11 years ago
measurement error
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Resolution: FIXED → INVALID
You need to log in before you can comment on or make changes to this bug.