503467 - Make the looping in nsCSSScanner::ParseNumber more readable

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Spun off of bug 498559.  This seems to be a performance wash, but not a slowdown, and I think this code is clearer.

Comment 1

[Alfred Kayser]

Looks much better than that complex loop.
Some small nits:
The 'float divisor' should be a double, just like all all numbers here.
and one could replace:
1200       value = value / 100.0f;
with:
1200       value /= 100.0f;

Comment 2

[Boris Zbarsky [:bzbarsky]]

> The 'float divisor' should be a double

Does that actually affect output?  Because it seems to make a noticeable performance difference when I measured...

Comment 3

[Alfred Kayser]

I assumed it would improve perf, but apparently not.

Another option is to prevent the float and division in the loop itself.
    int fracDigits = 0;
    do {
      fracPart = 10 * fracPart + CHAR_TO_DIGIT(c);
      fracDigits --;
      c = Read();
      // The IsDigit check will do the right thing even if Read() returns < 0
    } while (IsDigit(c));
    fracPart *= pow(10.0, fracDigits);

Again, no sure whether this improves performance, or how it important this is ;-)

Comment 4

[Boris Zbarsky [:bzbarsky]]

I doubt that would help much; pow() is not exactly cheap either.  I can test, though.

For what it's worth, from my reading of the generated assembly at least with g++ the #1 thing that would help is not doing a function call on every iteration.  g++ spills fracPart and intPart back to the stack before every Read() call (because it can't depend on the callee not trashing registers, presumably).  The corresponding per-loop reads off the stack and spills to the stack, instead of just doing all the work in the xmm registers, is by far the biggest remaining bottleneck in this function, from what I can tell.  Presumably we at least hit the L1 cache on this stuff, but still...

I'm not quite sure whether it's worth micro-optimizing this more a this point, though.  For reference, current trunk (at least once tracemonkey merges) is about 3x faster than Webkit and 5x faster than Opera (and 4x faster than Gecko 1.9.1) on the "set style.top a bunch of times" testcase, and the time under this function is only about 18% of the total time now.  That includes the unit string-gathering, etc...  It still shows up some in profiles, but at about the 2-3% level.  So ability to squeeze more real-life mileage out of this code is pretty limited until we make other things faster.  ;)

Comment 5

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

Comment on attachment 387820 [details] [diff] [review]
Like so

>+    if (gotE) {
>+      c = Read();
>+      NS_ASSERTION(IsDigit(c), "How did we get here?");
>+      do {
>+        exponent = 10*exponent + CHAR_TO_DIGIT(c);
>+        c = Read();
>+        // The IsDigit check will do the right thing even if Read() returns < 0
>+      } while (IsDigit(c));
>     }

Since you only make gotE true in one place, seems like you may as well move this in to the block where you set gotE to true and lose the if (gotE) condition.

r=dbaron

Comment 6

[Boris Zbarsky [:bzbarsky]]

Made that change and pushed http://hg.mozilla.org/mozilla-central/rev/ea46b7fe6320