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.

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.

Well, let me make a try :)

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.

(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.

Alexandre are you still working on this?

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 ...

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

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.

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 :)

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

Looking at awfy, we win more on just JM+TI compared to ION+JM+TI. We should investigate this.