Closed Bug 1331136 Opened 3 years ago Closed 3 years ago

CacheIR: Failing to attach StringChar in octance pdfjs

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: evilpie, Assigned: evilpie)

References

(Blocks 2 open bugs)

Details

(Keywords: perf, Whiteboard: [qf-])

Attachments

(1 file, 1 obsolete file)

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.
I'll mark this pro-actively P2. Might be good to get it fixed in the next iteration.
Blocks: jsperf
Keywords: perf
Priority: -- → P2
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.
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.
Priority: P2 → P1
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.
Assignee: nobody → evilpies
Status: NEW → ASSIGNED
Attachment #8846197 - Flags: review?(jdemooij)
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.
Attachment #8846197 - Flags: review?(jdemooij) → review+
(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...
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/323c768fdc43
Handle more StringChar cases with ropes in CacheIR. r=jandem
https://hg.mozilla.org/mozilla-central/rev/323c768fdc43
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Adding needinfo to evilpie in order to make sure this doesn't get forgotten.
Flags: needinfo?(evilpies)
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)
Attachment #8846197 - Attachment is obsolete: true
Flags: needinfo?(evilpies)
Attachment #8851150 - Flags: review?(jdemooij)
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!
Attachment #8851150 - Flags: review?(jdemooij) → review+
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a2921ee62c2
Handle more StringChar cases with ropes in CacheIR. r=jandem
https://hg.mozilla.org/mozilla-central/rev/5a2921ee62c2
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.