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)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla1.9.1
People
(Reporter: brendan, Assigned: gal)
Details
Attachments
(1 file, 4 obsolete files)
4.74 KB,
patch
|
brendan
:
review+
|
Details | Diff | 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)
Reporter | ||
Comment 1•16 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•16 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•16 years ago
|
||
The r = VALUE_TO_ID has to be fixed, too. The subsequent call to get(&r) isn't valid.
Reporter | ||
Comment 4•16 years ago
|
||
Attachment #336449 -
Attachment is obsolete: true
Assignee | ||
Updated•16 years ago
|
Summary: Trace slow array get/set of indexed props → TM: Trace slow array get/set of indexed props
Reporter | ||
Updated•16 years ago
|
Assignee: general → gal
Assignee | ||
Comment 5•16 years ago
|
||
Regresses performance of 3d-cube. v8/crypt traces with this patch but yields incorrect results.
Updated•16 years ago
|
Priority: -- → P3
Target Milestone: mozilla1.9.1b1 → mozilla1.9.1
Assignee | ||
Comment 6•16 years ago
|
||
Attachment #336622 -
Attachment is obsolete: true
Assignee | ||
Comment 7•16 years ago
|
||
Attachment #338594 -
Attachment is obsolete: true
Assignee | ||
Comment 8•16 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/ad95331b38ea
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Flags: in-testsuite-
Flags: in-litmus-
Assignee | ||
Comment 9•16 years ago
|
||
Patch doesn't work properly for float array indexes.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 10•16 years ago
|
||
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•16 years ago
|
Attachment #338733 -
Flags: review?(brendan) → review+
Reporter | ||
Comment 11•16 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•16 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/e51d84e91188 http://hg.mozilla.org/tracemonkey/rev/13e1792d5a87
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Comment 13•16 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.
Description
•