812446 - JM: getelem for string does not convert the index to an int.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

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.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Simple benchmark.

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

[Alexandre BM (:rednaks)]

Well, let me make a try :)

Comment 3

[Tom S [:evilpie]]

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.

Comment 4

[Nicolas B. Pierron [:nbp]]

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

Comment 5

[Tom S [:evilpie]]

Alexandre are you still working on this?

Comment 6

[Alexandre BM (:rednaks)]

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

Comment 7

[Tom S [:evilpie]]

Attachment: v1

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 8

[Nicolas B. Pierron [:nbp]]

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.

Comment 9

[Alexandre BM (:rednaks)]

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

Comment 10

[Tom S [: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

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f99c04a0afc5

Comment 12

[Tom S [:evilpie]]

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