Closed Bug 498559 Opened 14 years ago Closed 13 years ago

[FIX]Speed up ParseNumber

Categories

(Core :: CSS Parsing and Computation, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(3 files, 1 obsolete file)

Attached file 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.
Keywords: perf
Attached patch Proposed fix (obsolete) — Splinter Review
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...
Attachment #383410 - Flags: superreview?
Attachment #383410 - Flags: review?
Attachment #383410 - Flags: superreview?(dbaron)
Attachment #383410 - Flags: superreview?
Attachment #383410 - Flags: review?(dbaron)
Attachment #383410 - Flags: review?
Duplicate of this bug: 399349
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.
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.
Attachment #383410 - Attachment is obsolete: true
Attachment #383483 - Flags: superreview?(dbaron)
Attachment #383483 - Flags: review?(dbaron)
Attachment #383410 - Flags: superreview?(dbaron)
Attachment #383410 - Flags: review?(dbaron)
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 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.
Attachment #383483 - Flags: superreview?(dbaron)
Attachment #383483 - Flags: review?(dbaron)
Attachment #383483 - Flags: review+
(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).
> 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.
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.
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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....
Blocks: 503467
Filed bug 503467 on rearranging the code.
You need to log in before you can comment on or make changes to this bug.