Closed
Bug 415455
Opened 17 years ago
Closed 17 years ago
Faster integer conversions in the interpreter
Categories
(Core :: JavaScript Engine, enhancement, P2)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla1.9beta5
People
(Reporter: igor, Assigned: igor)
References
Details
Attachments
(3 files, 4 obsolete files)
Currently the macro FETCH_INT from jsinterp.c contains the following code:
#define FETCH_INT(cx, n, i) \
JS_BEGIN_MACRO \
jsval v_ = FETCH_OPND(n); \
if (JSVAL_IS_INT(v_)) { \
i = JSVAL_TO_INT(v_); \
} else { \
SAVE_SP_AND_PC(fp); \
ok = js_ValueToECMAInt32(cx, v_, &i); \
if (!ok) \
goto out; \
} \
JS_END_MACRO
That is, the code calls a relatively slow js_ValueToECMAInt32 even for doubles values.
Since the various bit manipulation bytecodes contains uses the macro, it means that a script with the corresponding bit operations would be penalized whenever the highest bit of the number would be set. Such numbers are converted to doubles in SpiderMonkey and FETCH_INT will take a slow path to access them. The same is applicable to FETCH_UINT macro.
It would be nice if SpiderMonkey would optimize the macros for the case of double values holding integers that fits 32 bits.
Assignee | ||
Comment 1•17 years ago
|
||
The fix optimized FETCH_(INT|UINT) to check for double values. Another optimization is in jsnum.c to take a fast path for double values holding int32 numbers. That optimization assumes that when d == -0.0, then in
int32 i;
...
i = (int32) d;
if ((jsdouble) i == d)
return i;
the if check would return true. It definitely works on Linux, but I would like to know if MSVC does the proper job here as well.
Attachment #301136 -
Flags: review?(brendan)
Assignee | ||
Comment 2•17 years ago
|
||
Here is SunSpider stats to show the difference with the patch applied. It has lines like:
md5: 1.23x as fast
sha1: 1.24x as fast
On the other hand there is a slowdown as well. To know if this is a real slowdown (perhaps due to increased code-size of the interpreter) or just a noise, it would be interesting to know the results on other computers.
Comment 4•17 years ago
|
||
Want the win but not the loss -- should be able to pinpoint why some tests slowed down, and maybe reorder jsval tag tests. Tell me if this is the wrong thing and I will r+, but for now I'm hoping for more data and a pure win.
/be
Comment 5•17 years ago
|
||
Sunspider coverage of jsinterp.c is attached. Lines 243-263 show the coverage of the macro FETCH_INT. I had to convert the macro to a function and tweak it a bit for the coverage tool to show the macro coverage details. Basically, ~96.8% of the cases (~67.2M/69M) are integers, ~2.81% of the cases (2.2M/69M) are doubles, and the rest ~0.4% (285K/69M) require js_ValueToNumber call. So, roughly 3% of the cases would be affected by this change, and 87% of this 3% goes through the suggested "fast" path. The absolute value of the affected cases is not a lot, it's < 2M altogether, but the imbalance between the branch probability makes it worth further investigation. I applied the patch to an older source base that I have and got almost the same results with md5 and sha1 the top winners ~1.3x while having some slowdowns. I also moved & replicated js_DoubleToECMAInt32 above to avoid some stores/loads - not much help. I'll investigate it further.
- moh
Assignee | ||
Comment 6•17 years ago
|
||
Comment on attachment 301136 [details] [diff] [review]
v1
A better patch will follow.
Attachment #301136 -
Flags: review?(brendan)
Updated•17 years ago
|
Flags: tracking1.9+ → blocking1.9+
Assignee | ||
Comment 7•17 years ago
|
||
Here is an updated patch. It tries to minimize the code bloat. In particular, it does not try to inline into the interpreter loop ToInt32(v) conversions when v is a double. Rather it changes the signatures of To(Int|Uint)32 conversion functions to return the converted value directly:
extern int32
js_ValueToECMAInt32(JSContext *cx, jsval *vp);
extern uint32
js_ValueToECMAUint32(JSContext *cx, jsval *vp);
For error reporting the code uses *vp which is set to JSVAL_NULL when the conversion fails. This way the compiler can keep the results in the registers.
Attachment #301136 -
Attachment is obsolete: true
Attachment #301137 -
Attachment is obsolete: true
Attachment #307485 -
Flags: review?(brendan)
Assignee | ||
Comment 8•17 years ago
|
||
These are the results comparing the trunk versus the effects of the patches from comment 7 and from bug 418641 comment 8. The total win is 2%.
Without the patch from bug 418641 the changes introduced in the patch for this bug adversely influence some benchmarks that never touches the opcode affected by this patch so there is no total win. The code shrink from bug 418641 removes that effect.
Assignee | ||
Comment 9•17 years ago
|
||
The previous version has a static assert right in the middle of the interpreter loop, this version moves it before js_Interpret.
Attachment #307485 -
Attachment is obsolete: true
Attachment #307551 -
Flags: review?(brendan)
Attachment #307485 -
Flags: review?(brendan)
Updated•17 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.9beta5
Comment 10•17 years ago
|
||
Comment on attachment 307551 [details] [diff] [review]
v3
>@@ -229,25 +229,22 @@ js_GetLengthProperty(JSContext *cx, JSOb
> *lengthp = obj->fslots[JSSLOT_ARRAY_LENGTH];
> return JS_TRUE;
> }
>
> JS_PUSH_SINGLE_TEMP_ROOT(cx, JSVAL_NULL, &tvr);
> id = ATOM_TO_JSID(cx->runtime->atomState.lengthAtom);
> ok = OBJ_GET_PROPERTY(cx, obj, id, &tvr.u.value);
> if (ok) {
>- /*
>- * Short-circuit, because js_ValueToECMAUint32 fails when called
>- * during init time.
>- */
> if (JSVAL_IS_INT(tvr.u.value)) {
> i = JSVAL_TO_INT(tvr.u.value);
> *lengthp = (jsuint)i; /* jsuint cast does ToUint32 */
> } else {
>- ok = js_ValueToECMAUint32(cx, tvr.u.value, (uint32 *)lengthp);
>+ *lengthp = (jsuint) js_ValueToECMAUint32(cx, &tvr.u.value);
Nit: no need for (jsuint) cast above, and it looks odd (though follows the more common style of space after cast) compared to the cast in the then clause.
> js_DoubleToECMAUint32(jsdouble d)
> {
>+ int32 i;
> JSBool neg;
>- jsdouble two32 = 4294967296.0;
>+ jsdouble two32;
>
>- if (!JSDOUBLE_IS_FINITE(d) || d == 0)
>+ if (!JSDOUBLE_IS_FINITE(d))
> return 0;
>
>+ /*
>+ * We check if d fits int32, not uint32 as all but ">>>" bit manipulation
>+ * bytecode stores the result as int, not uint. When the result does not
>+ * fit int jsval, it will be stored as a negative double.
s/check if/check whether/
s/not uint32/not uint32,/
s/all but/all but the/
r=me and go for approval1.9? with these picked. Thanks,
/be
Attachment #307551 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 11•17 years ago
|
||
The new version addresses the nits from comment 10. Asking for a+ as it improves on Linux sunspider results by 2%.
Attachment #307551 -
Attachment is obsolete: true
Attachment #307770 -
Flags: review+
Attachment #307770 -
Flags: approval1.9?
Comment on attachment 307770 [details] [diff] [review]
v4
You don't need approval, this is a blocker: land at will!
Attachment #307770 -
Flags: approval1.9?
Assignee | ||
Comment 13•17 years ago
|
||
I checked in the patch from comment 11 to the trunk:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1204839600&maxdate=1204839728&who=igor%25mir2.org
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Flags: in-testsuite-
Flags: in-litmus-
You need to log in
before you can comment on or make changes to this bug.
Description
•