Closed Bug 402491 Opened 17 years ago Closed 17 years ago

js_NumberToString should take a fast path for integers

Categories

(Core :: JavaScript Engine, defect, P4)

x86
Linux
defect

Tracking

()

RESOLVED INVALID

People

(Reporter: bzbarsky, Unassigned)

References

Details

(Keywords: perf)

Attachments

(1 file)

Brendan says that js_NumberToString can be a lot faster if the number happens to be an integer.  Since it often is (e.g. any sort of DHTML animation, typically), it might be worth optimizing this case.
Flags: blocking1.9?
Hmm.  Perhaps the testcase in bug 229391 is not using integers?  It does apply a rotation matrix and then projection to the cube, so might well not be ending up with integer pixels.  :(
Hi Boris
I'll check into that within the next few days, but from what I remember I think I already round everything before I set the the style properties.
Simon (creator of testcase in bug 229391)
Moving to blocking to capture perf push...
Flags: blocking1.9? → blocking1.9+
I hacked this brutally, so it's probably at least partly incorrect.  Should still be good enough as a simplified testcase to investigate the performance of spidermonkey here.
We should fix this for 1.9, but without a patch in sight....  P4?  I'll come back to this if I get some time....
Priority: -- → P4
I created a slightly different test case. Now you can render the cube either with no rounding before setting the style attributes or with rounding.

http://www.speich.net/computer/moztesting.php -> js_NumberToString should take a fast path for integers

no rounding:
Elapsed time: 3'704 ms
Average per loop: 74.08 ms
Fastest loop: 62 ms
Slowest loop: 109 ms

rounding:
Elapsed time: 3'189 ms
Average per loop: 63.78 ms
Fastest loop: 46 ms
Slowest loop: 94 ms

On average we gain 10ms by rounding numbers first before setting style attribute.
Which essentially invalidates this bug, doesn't it?  If rounding is improving performance by that much, then surely we -are- taking a fastpath for integers as the code seems to indicate.  Igor's doubles perf patch may help more, though (I forget bug #)
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → INVALID
I'm a little confused here. Doesn't this confirm that js_NumberToString is faster if the number happens to be an integer? Anyway, to make sure it's clear what I did:
No rounding means: El.style.left = x + "px";
Rounding: El.style.left =  Math.round(x) + "px";

The original test case for bug 229391 rounded only the position of the corner pixels, but not the positions of the line drawing pixels between the corners.
This bug was about making sure that js_NumberToString doesn't take the slow double-to-string path if the number is an integer.  Which it doesn't, as the test shows.

So yeah, the bug is invalid or worksforme or whatever.  But making double-to-string faster would be nice, yes...

What would be even better is if the CSSOM API didn't suck so much.  :(  Right now we do double-to-string in the JS engine, then string-to-double in the CSS.  I seem to recall the combination being on the order of 20% of the time on this testcase.
(In reply to comment #10) 
> What would be even better is if the CSSOM API didn't suck so much.  :(  Right
> now we do double-to-string in the JS engine, then string-to-double in the CSS. 
> I seem to recall the combination being on the order of 20% of the time on this
> testcase.
> 
Is there a bug on that?

A bug on which?  Getting the DOM spec changed?
(In reply to comment #12)
> A bug on which?  Getting the DOM spec changed?
> 

Yep - in making the web suck less :-P.  Sorry dumb question.
(In reply to comment #10)
> What would be even better is if the CSSOM API didn't suck so much.  :(  Right
> now we do double-to-string in the JS engine, then string-to-double in the CSS. 

Has implementing pixelLeft, pixelTop, posLeft and posTop ( http://msdn2.microsoft.com/ms531143 ) been considered? Opera already has copied these properties from IE.
I've always found it pretty stupid to append "px" or some other unit to the coordinates just so the CSS parser can rip it off again.
We've considered it on and off, yes...
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: