[FIX]Speed up ParseNumber

RESOLVED FIXED

Status

()

RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(Blocks: 2 bugs, {perf})

Trunk
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

10 years ago
Created attachment 383408 [details]
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.
(Assignee)

Updated

10 years ago
Keywords: perf
(Assignee)

Comment 1

10 years ago
Created attachment 383410 [details] [diff] [review]
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...
Attachment #383410 - Flags: superreview?
Attachment #383410 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #383410 - Flags: superreview?(dbaron)
Attachment #383410 - Flags: superreview?
Attachment #383410 - Flags: review?(dbaron)
Attachment #383410 - Flags: review?

Updated

10 years ago
Duplicate of this bug: 399349

Comment 3

10 years ago
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.
(Assignee)

Comment 4

10 years ago
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.
(Assignee)

Comment 5

10 years ago
Created attachment 383483 [details] [diff] [review]
Updated with that test; no merging to eliminate main loop yet.
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)
(Assignee)

Comment 6

10 years ago
Created attachment 383484 [details] [diff] [review]
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 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).
(Assignee)

Comment 9

9 years ago
> 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
Last Resolved: 9 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....
(Assignee)

Updated

9 years ago
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.