Closed Bug 453261 Opened 16 years ago Closed 16 years ago

TM: Trace slow array get/set of indexed props

Categories

(Core :: JavaScript Engine, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1

People

(Reporter: brendan, Assigned: gal)

Details

Attachments

(1 file, 4 obsolete files)

Attached patch patch (obsolete) — Splinter Review
Like so. Layering of record_JSOP_GETELEM on top of elem makes for different logic and redundant test of r's type, compared to record_JSOP_SETELEM, but in both cases the generic indexing is done by Any_[gs]etelem while the named access is done by Any_[gs]etprop (renamed from ...elem to avoid confusion, I hope!).

/be
Attachment #336449 - Flags: review?(gal)
Comment on attachment 336449 [details] [diff] [review]
patch

Argh, regressed 3d-cube. Fixing...

/be
Attachment #336449 - Flags: review?(gal) → review-
Comment on attachment 336449 [details] [diff] [review]
patch

>@@ -4122,30 +4122,38 @@ TraceRecorder::record_JSOP_GETELEM()
> 
>         LIns* args[] = { f2i(get(&r)), get(&l), cx_ins };
>         LIns* unitstr_ins = lir->insCall(F_String_getelem, args);
>         guard(false, lir->ins_eq0(unitstr_ins), MISMATCH_EXIT);
>         set(&l, unitstr_ins);
>         return true;
>     }
> 
>-    if (!JSVAL_IS_PRIMITIVE(l) && JSVAL_IS_STRING(r)) {
>+    if (!JSVAL_IS_PRIMITIVE(l) &&
>+        (JSVAL_IS_STRING(r) || (JSVAL_IS_INT(r) && !OBJ_IS_DENSE_ARRAY(cx, JSVAL_TO_OBJECT(l))))) {

The right of the || is to blame, if I comment out the ||... then 3d-cube is fast again. Why? We should not be slowing down due to less aborting. Need to study debug spew with and without this patch. If someone else beats me to it, please fix and steal this bug!

/be
The r = VALUE_TO_ID has to be fixed, too. The subsequent call to get(&r) isn't valid.
Attached patch better attempt (obsolete) — Splinter Review
Attachment #336449 - Attachment is obsolete: true
Summary: Trace slow array get/set of indexed props → TM: Trace slow array get/set of indexed props
Assignee: general → gal
Regresses performance of 3d-cube. v8/crypt traces with this patch but yields incorrect results.
Priority: -- → P3
Target Milestone: mozilla1.9.1b1 → mozilla1.9.1
Attached patch refreshed patch (obsolete) — Splinter Review
Attachment #336622 - Attachment is obsolete: true
Attached patch refreshed, again (obsolete) — Splinter Review
Attachment #338594 - Attachment is obsolete: true
http://hg.mozilla.org/tracemonkey/rev/ad95331b38ea
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
Flags: in-litmus-
Patch doesn't work properly for float array indexes.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Use floating point in the builtin by default but specialize to int if possible.
Attachment #338597 - Attachment is obsolete: true
Attachment #338733 - Flags: review?(brendan)
Attachment #338733 - Flags: review?(brendan) → review+
Comment on attachment 338733 [details] [diff] [review]
Fix floating-point indexes.

>@@ -4476,7 +4490,8 @@
>     }
> 
>     if (!JSVAL_IS_PRIMITIVE(l) &&
>-        (JSVAL_IS_STRING(r) || (JSVAL_IS_INT(r) && !OBJ_IS_DENSE_ARRAY(cx, JSVAL_TO_OBJECT(l))))) {
>+        (JSVAL_IS_STRING(r) || 
>+        (isNumber(r) && (!JSVAL_IS_INT(r) || !OBJ_IS_DENSE_ARRAY(cx, JSVAL_TO_OBJECT(l)))))) {

Last line of multiline condition needs to indent one space more, since it is within the parentheses that start with the ( to the left of JSVAL_IS_STRING(r).

r=me with that nit picked. Thanks!

/be
http://hg.mozilla.org/tracemonkey/rev/e51d84e91188

http://hg.mozilla.org/tracemonkey/rev/13e1792d5a87
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
test already in js1_8_1/trace/trace-test.js
Flags: in-testsuite- → in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: