Closed
Bug 812446
Opened 12 years ago
Closed 12 years ago
JM: getelem for string does not convert the index to an int.
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: nbp, Assigned: evilpie)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
563 bytes,
application/x-javascript
|
Details | |
3.82 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
Using the SPS profiler and the JIT Inspector, Brian and I noticed a defect which account for about ~19% of the time of pdfjs' octane benchmark.
The Gecko profiler showed that str_resolve is called for adding properties to a String when the string is accessed through a GetElem under Type1Parser_extractFontProgram. The JIT inspector pin-point the usage to be mostly coming from “var c = eexecStr[i];”. The detail information of the JIT Inspector highlight the fact that “i” is seen as a double by TI.
The problem seems to be that “parseInt” returns a NaN when it goes a non-numerical token and the NaN flows into the loop variable counter which is also used to access strings.
Reporter | ||
Comment 1•12 years ago
|
||
This test case can be used to highlight if the bug is fixed or not.
Adding a print() arround str[i] might help for checking if the output is correct or not.
To check if the issue is present, one can run the following commands under gdb
$ gdb --args ./js --no-ion ./bug812446.js
(gdb) b str_resolve
// set a break point to str_resolve
(gdb) ignore 1 50
// ignore the first iterations which are run in the interpreter
(gdb) r
// Start the execution of the test case
(gdb) bt
// Check the backtrace.
When the benchmark run with --no-ion, we obtain a backtrace implying that JäegerMonkey does not optimize this case:
(gdb) bt
#0 str_resolve (cx=0xee7410, obj=..., id=..., flags=1, objp=...) at /home/nicolas/mozilla/ionmonkey/js/src/jsstr.cpp:392
#1 0x00000000005843b9 in CallResolveOp (cx=0xee7410, obj=..., id=..., flags=1, objp=..., propp=..., recursedp=0x7fffffff92ff)
at /home/nicolas/mozilla/ionmonkey/js/src/jsobj.cpp:4056
#2 0x0000000000584782 in LookupPropertyWithFlagsInline (cx=0xee7410, obj=..., id=..., flags=65535, objp=..., propp=...)
at /home/nicolas/mozilla/ionmonkey/js/src/jsobj.cpp:4108
#3 0x0000000000585a08 in js_GetPropertyHelperInline (cx=0xee7410, obj=..., receiver=..., id_=..., getHow=0, vp=...)
at /home/nicolas/mozilla/ionmonkey/js/src/jsobj.cpp:4311
#4 0x00000000005861b9 in js::baseops::GetProperty (cx=0xee7410, obj=..., receiver=..., id=..., vp=...)
at /home/nicolas/mozilla/ionmonkey/js/src/jsobj.cpp:4406
#5 0x0000000000408f1a in JSObject::getGeneric (cx=0xee7410, obj=..., receiver=..., id=..., vp=...)
at /home/nicolas/mozilla/ionmonkey/js/src/jsobjinlines.h:171
#6 0x000000000042f199 in JSObject::getProperty (cx=0xee7410, obj=..., receiver=..., name=0x7fffef229e00, vp=...)
at /home/nicolas/mozilla/ionmonkey/js/src/jsobjinlines.h:182
#7 0x00000000009b5fa9 in js::GetObjectElementOperation (cx=0xee7410, op=JSOP_GETELEM, obj=..., rref=..., res=...)
at /home/nicolas/mozilla/ionmonkey/js/src/jsinterpinlines.h:750
#8 0x00000000009b641c in js::GetElementOperation (cx=0xee7410, op=JSOP_GETELEM, lref=..., rref=..., res=...)
at /home/nicolas/mozilla/ionmonkey/js/src/jsinterpinlines.h:801
#9 0x00000000009b6a2b in js::mjit::stubs::GetElem (f=...) at /home/nicolas/mozilla/ionmonkey/js/src/methodjit/StubCalls.cpp:105
#10 0x00007ffff7f69226 in ?? ()
When the benchmark is run with --no-jm, we obtain a backtrace which imply that IonMonkey does not handle this case either:
(gdb) bt
#0 str_resolve (cx=0xee7350, obj=..., id=..., flags=1, objp=...) at /home/nicolas/mozilla/ionmonkey/js/src/jsstr.cpp:392
#1 0x00000000005843b9 in CallResolveOp (cx=0xee7350, obj=..., id=..., flags=1, objp=..., propp=..., recursedp=0x7fffffff92af)
at /home/nicolas/mozilla/ionmonkey/js/src/jsobj.cpp:4056
#2 0x0000000000584782 in LookupPropertyWithFlagsInline (cx=0xee7350, obj=..., id=..., flags=65535, objp=..., propp=...)
at /home/nicolas/mozilla/ionmonkey/js/src/jsobj.cpp:4108
#3 0x0000000000585a08 in js_GetPropertyHelperInline (cx=0xee7350, obj=..., receiver=..., id_=..., getHow=0, vp=...)
at /home/nicolas/mozilla/ionmonkey/js/src/jsobj.cpp:4311
#4 0x00000000005861b9 in js::baseops::GetProperty (cx=0xee7350, obj=..., receiver=..., id=..., vp=...)
at /home/nicolas/mozilla/ionmonkey/js/src/jsobj.cpp:4406
#5 0x0000000000408f1a in JSObject::getGeneric (cx=0xee7350, obj=..., receiver=..., id=..., vp=...)
at /home/nicolas/mozilla/ionmonkey/js/src/jsobjinlines.h:171
#6 0x000000000042f199 in JSObject::getProperty (cx=0xee7350, obj=..., receiver=..., name=0x7fffef229e20, vp=...)
at /home/nicolas/mozilla/ionmonkey/js/src/jsobjinlines.h:182
#7 0x0000000000542599 in js::GetObjectElementOperation (cx=0xee7350, op=JSOP_GETELEM, obj=..., rref=..., res=...)
at /home/nicolas/mozilla/ionmonkey/js/src/jsinterpinlines.h:750
#8 0x0000000000542a0c in js::GetElementOperation (cx=0xee7350, op=JSOP_GETELEM, lref=..., rref=..., res=...)
at /home/nicolas/mozilla/ionmonkey/js/src/jsinterpinlines.h:801
#9 0x0000000000554354 in js::GetElement (cx=0xee7350, lref=..., rref=..., vp=...)
at /home/nicolas/mozilla/ionmonkey/js/src/jsinterp.cpp:3968
#10 0x00007ffff7f6908c in ?? ()
If the output is correct, we should see a *significant drop* in term of execution time of the benchmark. Running the JS shell with the “ -b ” option will output the time used to run the benchmark.
Comment 2•12 years ago
|
||
Well, let me make a try :)
Reporter | ||
Updated•12 years ago
|
Assignee: general → s
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•12 years ago
|
||
Nicolas: I think your benchmark has a small bug, I think you forgot the parentheses in "if (i == (i | 0))".
This made the return typeset undefined | string, which is harder to optimize.
Reporter | ||
Comment 4•12 years ago
|
||
(In reply to Tom Schuster [:evilpie] from comment #3)
> Nicolas: I think your benchmark has a small bug, I think you forgot the
> parentheses in "if (i == (i | 0))".
Indeed.
Assignee | ||
Comment 5•12 years ago
|
||
Alexandre are you still working on this?
Comment 6•12 years ago
|
||
Hi Tom,
I'd like to but i'm blocking and I didn't find time to focus on the problem. Unfortunatly I'll be available only after 3 weeks so if you want to take it ...
Assignee | ||
Updated•12 years ago
|
Assignee: s → evilpies
Assignee | ||
Comment 7•12 years ago
|
||
So this patch fixes a few related things at once. In GetElementOperation it makes no sense to only handle the int32 index case for strings, this should at least prevent str_resolve in JM + interp. The rest is basically just allowing charAt, charCodeAt and string[index] with doubles.
Just for fun some numbers.
The broken benchmark (return result including undefined)
387ms 870ms
The fixed benchmark
15ms 431ms
Attachment #690910 -
Flags: review?
Assignee | ||
Updated•12 years ago
|
Attachment #690910 -
Flags: review? → review?(nicolas.b.pierron)
Reporter | ||
Comment 8•12 years ago
|
||
Comment on attachment 690910 [details] [diff] [review]
v1
Review of attachment 690910 [details] [diff] [review]:
-----------------------------------------------------------------
This is a good patch, which fix GetElementOperation for string by using a function which recover an index from both an int or a double.
This patch also accept double index input for path producing a MCharCodeAt MIR which are using MToInt32 for converting double inputs to int.
Attachment #690910 -
Flags: review?(nicolas.b.pierron) → review+
Comment 9•12 years ago
|
||
I can't believe that I had the same solution for GetElem (no ion) and I didn't attach it ! maybe this is due to my misunderstanding of the bug ... I have to be more confident next time :s
Thanks nbp, evilpie :)
Assignee | ||
Comment 10•12 years ago
|
||
Oh, that is sad :(. Please show us what you did next time, so we can help you out.
You will surely find something interesting to work on after these 3 weeks.
Nicolas:
Yep! Thanks for review.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f99c04a0afc5
Comment 11•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Assignee | ||
Comment 12•12 years ago
|
||
Looking at awfy, we win more on just JM+TI compared to ION+JM+TI. We should investigate this.
You need to log in
before you can comment on or make changes to this bug.
Description
•