Closed Bug 559715 Opened 12 years ago Closed 12 years ago

Micro-optimize nsCSSPropertySet

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: zwol, Assigned: zwol)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
The size of the generated code for nsCSSPropertySet can be cut approximately in half by using 'unsigned long' instead of PRUint8 for storage (I deliberately use this unwrapped type so that it will be register-sized on both ILP32 and LP64 builds), and explicitly converting method arguments from nsCSSProperty to size_t before using them as indices.  The latter is the more important change; it allows the compiler to use unsigned rather than signed arithmetic (the nsCSSProperty enumeration includes the value -1, so the whole type has to be signed).  Here's what the generated code for one method looks like before the change:

_ZN16nsCSSPropertySetImE11AddPropertyE13nsCSSProperty:
        leal    63(%rsi), %edx
        testl   %esi, %esi
        movl    %esi, %eax
        cmovns  %esi, %edx
        sarl    $31, %eax
        shrl    $26, %eax
        sarl    $6, %edx
        leal    (%rsi,%rax), %ecx
        movslq  %edx,%rdx
        andl    $63, %ecx
        subl    %eax, %ecx
        movl    $1, %eax
        salq    %cl, %rax
        orq     %rax, (%rdi,%rdx,8)
        ret

and after:

_ZN16nsCSSPropertySetImE11AddPropertyE13nsCSSProperty:
        movslq  %esi,%rcx
        movl    $1, %eax
        movq    %rcx, %rdx
        andl    $63, %ecx
        shrq    $6, %rdx
        salq    %cl, %rax
        orq     %rax, (%rdi,%rdx,8)
        ret
Attachment #439424 - Flags: review?(dbaron)
I also converted PRBool to bool in this code (just for tidiness' sake -- it doesn't make a codegen difference), which means some '!= 0's are now unnecessary, but I think it reads better with them there.
Attached patch slightly revised patch (obsolete) — Splinter Review
Miss one lousy cast and it all blows up.
Attachment #439424 - Attachment is obsolete: true
Attachment #440105 - Flags: review?(dbaron)
Attachment #439424 - Flags: review?(dbaron)
Comment on attachment 440105 [details] [diff] [review]
slightly revised patch

r=dbaron if you replace those tabs you introduced with appropriate numbers of spaces
Attachment #440105 - Flags: review?(dbaron) → review+
Tabs removed from my copy; will wait to land until the tree is no longer metered.
as requested.
Attachment #440105 - Attachment is obsolete: true
Attachment #441060 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/11b94fd78548 (pushed by Brandon Sterne along with a bunch of other stuff)
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.