Bug 136936 – CSS JIT: Don't add backtracking register count to minimum register count if it's not necessary

• Status: RESOLVED
• Resolution: WONTFIX
• Component: CSS

[Yusuke Suzuki]

minimumRequiredRegisterCount always contains `BacktrackingRegister`.
However when the fragment is not inside InChainWithDescendantTail, this register is not allocated.

So adding `backtrackingRegisterRequirements` only if it's necessary.

[Yusuke Suzuki]

Created attachment 238348 [details]
Patch

[Benjamin Poulain]

That would be useful for ARMv7. But we would need to re-check all the generators because everything was coded with the assumption of 6 for the rightmost fragment.

For example, I believe :nth-child() uses 6 registers on x86 when doing style resolution for the rightmost selector. I can check that tomorrow.

In any case, we should land this after https://bugs.webkit.org/show_bug.cgi?id=136933 to have better chance of catching mistakes.

[Yusuke Suzuki]

(In reply to comment #2)
> That would be useful for ARMv7. But we would need to re-check all the generators because everything was coded with the assumption of 6 for the rightmost fragment.
> 
> For example, I believe :nth-child() uses 6 registers on x86 when doing style resolution for the rightmost selector. I can check that tomorrow.
> 
> In any case, we should land this after https://bugs.webkit.org/show_bug.cgi?id=136933 to have better chance of catching mistakes.

You're right. That patch[1] points out invalid assumption by raising assertion failures.
It makes this patch safer.

[1]: https://bugs.webkit.org/show_bug.cgi?id=136933

[Yusuke Suzuki]

To ensure this patch pass the test under the https://bugs.webkit.org/show_bug.cgi?id=136933, I'll re-upload the same patch.

[Yusuke Suzuki]

Created attachment 238633 [details]
Patch

[Benjamin Poulain]

Comment on attachment 238633 [details]
Patch

Clearing the review flag.

The patch is likely correct, but I don't think the gain/risk is worth it at this very moment.

My rationale:

All the code has been verified for 6 registers for the rightmost fragment and 5 for backtracking. Verifying all the generators for 5 registers would be very time consuming, it would probably take me an entire day of review.

We are on the verge of adding complex new capabilities to the engine. There are at least two features that will need great attention: 1) matching multiple pseudo elements with :matches() 2) Multilevel backtracking for :nth-child(), :matches() and :not().

The minus side of not including this patch is some performance impact on ARMv7. But even today without it we are several times faster than any competing engine.

[Yusuke Suzuki]

(In reply to comment #6)
> (From update of attachment 238633 [details])
> Clearing the review flag.
> 
> The patch is likely correct, but I don't think the gain/risk is worth it at this very moment.
> 
> My rationale:
> 
> All the code has been verified for 6 registers for the rightmost fragment and 5 for backtracking. Verifying all the generators for 5 registers would be very time consuming, it would probably take me an entire day of review.
> 
> We are on the verge of adding complex new capabilities to the engine. There are at least two features that will need great attention: 1) matching multiple pseudo elements with :matches() 2) Multilevel backtracking for :nth-child(), :matches() and :not().
> 
> The minus side of not including this patch is some performance impact on ARMv7. But even today without it we are several times faster than any competing engine.

Make sense. Yup, that's reasonable decision.
So closing this issue :)