TM: Trace slow array get/set of indexed props

RESOLVED FIXED in mozilla1.9.1

Status

()

Core
JavaScript Engine
P3
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: brendan, Assigned: gal)

Tracking

Trunk
mozilla1.9.1
Points:
---
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

10 years ago
Created attachment 336449 [details] [diff] [review]
patch

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)
(Reporter)

Comment 1

10 years ago
Comment on attachment 336449 [details] [diff] [review]
patch

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

/be
Attachment #336449 - Flags: review?(gal) → review-
(Reporter)

Comment 2

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

Comment 3

10 years ago
The r = VALUE_TO_ID has to be fixed, too. The subsequent call to get(&r) isn't valid.
(Reporter)

Comment 4

10 years ago
Created attachment 336622 [details] [diff] [review]
better attempt
Attachment #336449 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Summary: Trace slow array get/set of indexed props → TM: Trace slow array get/set of indexed props
(Reporter)

Updated

10 years ago
Assignee: general → gal
(Assignee)

Comment 5

10 years ago
Regresses performance of 3d-cube. v8/crypt traces with this patch but yields incorrect results.

Updated

10 years ago
Priority: -- → P3
Target Milestone: mozilla1.9.1b1 → mozilla1.9.1
(Assignee)

Comment 6

10 years ago
Created attachment 338594 [details] [diff] [review]
refreshed patch
Attachment #336622 - Attachment is obsolete: true
(Assignee)

Comment 7

10 years ago
Created attachment 338597 [details] [diff] [review]
refreshed, again
Attachment #338594 - Attachment is obsolete: true
(Assignee)

Comment 8

10 years ago
http://hg.mozilla.org/tracemonkey/rev/ad95331b38ea
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Flags: in-testsuite-
Flags: in-litmus-
(Assignee)

Comment 9

10 years ago
Patch doesn't work properly for float array indexes.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 10

10 years ago
Created attachment 338733 [details] [diff] [review]
Fix floating-point indexes.

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)
(Reporter)

Updated

10 years ago
Attachment #338733 - Flags: review?(brendan) → review+
(Reporter)

Comment 11

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

Comment 12

10 years ago
http://hg.mozilla.org/tracemonkey/rev/e51d84e91188

http://hg.mozilla.org/tracemonkey/rev/13e1792d5a87
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED

Comment 13

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