559715 - Micro-optimize nsCSSPropertySet

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Zack Weinberg (:zwol)]

Attachment: patch

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

Comment 1

[Zack Weinberg (:zwol)]

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.

Comment 2

[Zack Weinberg (:zwol)]

Attachment: slightly revised patch

Miss one lousy cast and it all blows up.

Comment 3

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

Comment on attachment 440105 [details] [diff] [review]
slightly revised patch

r=dbaron if you replace those tabs you introduced with appropriate numbers of spaces

Comment 4

[Zack Weinberg (:zwol)]

Tabs removed from my copy; will wait to land until the tree is no longer metered.

Comment 5

[Zack Weinberg (:zwol)]

Attachment: patch without tabs

as requested.

Comment 6

[Zack Weinberg (:zwol)]

http://hg.mozilla.org/mozilla-central/rev/11b94fd78548 (pushed by Brandon Sterne along with a bunch of other stuff)