498559 - [FIX]Speed up ParseNumber

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Microbenchmark

I finally got tired of .top/.left setting showing up in profiles and decided to dig into it today.  Microbenchmark that measures setting .top attached.  As usual, it shows a bunch of time spent in ParseNumber and the things that calls (ToFloat and ToString on nsString).

Since those do a bunch of edge case stuff we don't need to worry about here, and look at what chars are coming in (even though we've already made sure that only digits/signs/'.'/e/E are coming through), it seemed worth it to try reimplementing by hand.  Patch to do that coming up.  It shaves off about 15% of the time on the microbenchmark, which translates to 3-4% of the time on the testcase in bug 497788, for example.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

If desired, I have a version of this patch that doesn't rearrange the order of the conditionals in the loop quite as much, but this setup seemed clearer to me...

Comment 3

[Alfred Kayser]

See bug 399349 for same testcase material, as requested by the reviewers over there. And indeed the logic of gotE, gotDot, and the loop can be improved to much cleaner.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Aha!  I knew I'd seen a patch from you working on this code, but for some reason that bug didn't turn up when I searched...

My patch does pass that test (which I'll include in the version I upload in a second).

You're saying that the gotDot/gotE/loop stuff can be improved over my last patch, I assume?  I'll see whether I can merge in the relevant parts of your patch.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated with that test; no merging to eliminate main loop yet.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: An attempt to merge in elimination of that main dispach loop; applies on top of the above

Not requesting review here, because performance compared to the previous patch is at best a wash and maybe even a tiny bit slower (hard to tell) in my testing.  Which is a little odd; I would have expected a speedup from this change due to the tighter loops and less branchiness....

So I'd prefer we take the other patch and worry about rearranging this separately.  David, feel free to review if you think the resulting code looks clearer than what the other patch produces.

Comment 7

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

Comment on attachment 383483 [details] [diff] [review]
Updated with that test; no merging to eliminate main loop yet.

>+    value *= pow(10.0, double(expSign * exponent));

I wonder how efficient pow() is for cases like this.  But it's a rare case, so probably fine to leave, at least for now.

>+    aToken.mInteger = PRInt32(sign * intPart);

seems like this should just be
  aToken.mInteger = PRInt32(value);
since value already holds sign * intPart (fracPart better be zero in this case!).


r=dbaron.  Sorry for the delay.

Comment 8

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

(In reply to comment #7)
> >+    aToken.mInteger = PRInt32(sign * intPart);
> 
> seems like this should just be
>   aToken.mInteger = PRInt32(value);
> since value already holds sign * intPart (fracPart better be zero in this
> case!).

Ignore this (per IRC).

Comment 9

[Boris Zbarsky [:bzbarsky]]

> I wonder how efficient pow() is for cases like this.  But it's a rare case, so
> probably fine to leave, at least for now.

Pretty much what I was thinking.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/e32a27173098 for the changes and the property database test and http://hg.mozilla.org/mozilla-central/rev/bea398847d9a for the other test.

I'll do some more testing on the loop rearrangement and spin off a separate bug as needed.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Er, and pushed http://hg.mozilla.org/mozilla-central/rev/a339159b39bf with some cleanup that was part of the review.  That's what I get for forgetting to qfold....

Comment 12

[Boris Zbarsky [:bzbarsky]]

Filed bug 503467 on rearranging the code.