CSS JIT: The backtracking register can be ignored from the minimumRegisterRequirements
Created attachment 238286 [details] Patch
Yusuke, could you please double check I am not missing any case? I'll add more testing infrastructure tomorrow but I would like multiple reviews on this.
rdar://problem/18368294
(In reply to comment #2) > Yusuke, could you please double check I am not missing any case? > > I'll add more testing infrastructure tomorrow but I would like multiple reviews on this. Sure. I'll check it now :)
Comment on attachment 238286 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238286&action=review Maybe I don't understand the issue's background enough, sorry, I can't see the rdar link... If I understand this issue correctly, I think there's 2 meanings. Please point it :) 1. When there's no :not, :-webkit-any or attributes, `backtrackingRegisterRequirements` is not added to the minimum requirements. However, seeing the comment on `minimumRequiredRegisterCount`, it contains `BacktrackingRegister`. So I think that `BacktrackingRegister` count is already added, right? 2. `BacktrackingRegister` count is always added to the minimum register requirements whenever it isn't used. I think this pointing is right. So to fix it, 1) changing `minimumRequiredRegisterCount` to 5, it doesn't contains `BacktrackingRegister`. 2) defining `minimum` as `minimumRequiredRegisterCount + backtrackingRegisterRequirements` after the `InChainWithDescendantTail` check. OR adding `backtrackingRegisterRequirements` to `minimum` inside the `InChainWithDescendantTail` check. (like your change!) Is it correct? > Source/WebCore/cssjit/SelectorCompiler.cpp:859 > + minimum += minimumRequiredRegisterCount; I think it would be `+= backtrackingRegisterRequirements`. Is it correct?
Comment on attachment 238286 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238286&action=review >> Source/WebCore/cssjit/SelectorCompiler.cpp:859 >> + minimum += minimumRequiredRegisterCount; > > I think it would be `+= backtrackingRegisterRequirements`. Is it correct? Arg, ffs. I did a last minute change and I messed up the copy-paste. I am glad I asked you for a review.
Created attachment 238289 [details] Patch
You are right, this is wrong as you pointed out, minimumRequiredRegisterCount already account for the backtracking register. Looks like :nth-child() uses 6 registers in some path then. I'll look into it and upload a new patch asap.
Created attachment 238292 [details] Patch
Comment on attachment 238292 [details] Patch The patch is for the branches. ToT will get a different patch, I'll work that out tomorrow. The test is for ToT and the branches.
Committed r173730: <http://trac.webkit.org/changeset/173730>
Closing: Patch submitted in the branch. Test submitted in ToT. Added some code to catch such case earlier next time: https://bugs.webkit.org/show_bug.cgi?id=136933 ToT does not need the extra register. There are two patch of maximum allocation: 1) currentElement 2) elementCounter 3) previousSibling 4) elementRareData 5) cachedChildIndex and 1) currentElement 2) elementCounter 3) checkingContext 4) childStyle 5) flags 6) isFirstChildStateFlagImmediate The second path is safe for now because it is only executed on for the rightmost element. By definition, it cannot have a backtracking register.