1331136 - CacheIR: Failing to attach StringChar in octance pdfjs

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Tom S [:evilpie]]

We are failing to attach StringChar 656 times at pdfjs.js:17083. This is the line headerString[i]. headerString is a non-linear string in this case. I just don't understand why we keep trying.

Comment 1

[Hannes Verschore [:h4writer]]

I'll mark this pro-actively P2. Might be good to get it fixed in the next iteration.

Comment 2

[Tom S [:evilpie]]

I looked into this, and something was nagging at me, but it took me a while to find. So we fail to attach the stub, this means we are going to execute the getChar operation under GetElement. So you would usually assume the first time this fails we have a linear string afterwards and the next time we end up in the GetElem IC we just attach the stub. But this doesn't happen, because of Bug 851064 we optimize for one level deep ropes. Which basically means we only every linearize the left or right side of the rope, but never the rope itself! So we can never attach the stub ... I am pretty sure this basic problem also affects Ion where we also just fail for ropes, but at least have an OOL stub for them.

I see 3 solutions to this:
1) Remove the optimization
2) Add the same optimization to the IC and Ion code
3) Explicitly linearize strings in Ion and/or CacheIR stub

All of these have upsides and downsides, 1) is probably the easiest and yields a win on pdf.js, but would probably regress the original benchmark. 2) More difficult but we could keep the win from bug 851064 3) Relatively easy, but makes for some weird performance characteristics. (i.e if we only ever execute this once we might be to eager etc.)

With disabling the optimization in JSString::getChar I see a pretty clear win in pdf.js from ~12300 to ~13600.

Comment 3

[Tom S [:evilpie]]

4) Only do this optimization in JSString::getChar when the rope side is less than some percent of the total length, we can be as conservative as we want here. We really just want to avoid the case where the complete string is just the left side plus a handful of characters on the right side, which happens when manually constructing a string like with  while (something) str += "a"; and then going through it from left to right.

I tried implementing this and it didn't see any huge change either way on Octane.

Comment 4

[Tom S [:evilpie]]

Attachment: Optimize StringChar when index is in the left side of a rope

This is a simple patch that just optimizes the case where the index is in the left side of the rope. Handling the right side means preserving the index register. pdf.js is extremely unstable on my machine, but I think this could increase our score by 500 in the best case scenario.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8846197 [details] [diff] [review]
Optimize StringChar when index is in the left side of a rope

Review of attachment 8846197 [details] [diff] [review]:
-----------------------------------------------------------------

Clever, thanks!

Please file a follow-up bug to handle the other cases in the CacheIR code. We could just do a callWithABI to a helper function that calls JSString::ensureLinear (it cannot GC) and it would be nice to have the IC support all cases.

::: js/src/jit/CacheIR.cpp
@@ +1228,5 @@
> +    // This follows JSString::getChar, otherwise we fail to attach getChar in a lot of cases.
> +    if (str->isRope()) {
> +        JSRope* rope = &str->asRope();
> +
> +        // Make sure the string is fully contained by the right side

Nit: maybe "// Make sure the left side contains the index." or something?

::: js/src/jit/MacroAssembler.cpp
@@ +1389,5 @@
>      MOZ_ASSERT(index != output);
>  
> +    movePtr(str, output);
> +
> +    // This is follows JSString::getChar.

Nit: This follows

@@ +1392,5 @@
> +
> +    // This is follows JSString::getChar.
> +    Label notRope;
> +    Address flags(str, JSString::offsetOfFlags());
> +    branchTest32(Assembler::NonZero, flags, Imm32(JSString::TYPE_FLAGS_MASK), &notRope);

We should probably add branchIfNotRope, similar to branchIfRope.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #5)
> We could just do a callWithABI to a helper function that calls
> JSString::ensureLinear

Or JSString::getChar...

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/323c768fdc43
Handle more StringChar cases with ropes in CacheIR. r=jandem

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/323c768fdc43

Comment 9

[Hannes Verschore [:h4writer]]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/93a2da765249ff49ff6a19491221f56261f5d224.

We got reports on failures on specific sites and after careful re-review noticed a problem in the patch.
The code assumes that the left child of a rope has the same encoding (1-byte/2-byte) as the rope itself. Which is not guaranteed to be correct. They can differ. With a dirty fix I was able to confirm that this fixes the issue here on those sites.

I already talked with Evilpie about this issue on irc and he agreed with the backout for now.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Merged backout https://hg.mozilla.org/mozilla-central/rev/93a2da765249

Comment 11

[Hannes Verschore [:h4writer]]

Adding needinfo to evilpie in order to make sure this doesn't get forgotten.

Comment 12

[Tom S [:evilpie]]

Attachment: v2 - Optimize StringChar when index is in the left side of a rope

Took a bit of head-scratching, but I managed to write a test for this. (The result char-codes weren't in the unitStaticTable and would thus fail and run in the VM again)

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8851150 [details] [diff] [review]
v2 - Optimize StringChar when index is in the left side of a rope

Review of attachment 8851150 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense, thanks for adding the test!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a2921ee62c2
Handle more StringChar cases with ropes in CacheIR. r=jandem

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5a2921ee62c2