Bug 132951 – CSS JIT: Apply backtracking optimization to adjacent backtracking

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Yusuke Suzuki]

Likewise Applying backtracking optimization done for descendant backtracking to adjacent backtracking.

[Yusuke Suzuki]

Created attachment 231508 [details]
Patch

[Yusuke Suzuki]

WIP

[Benjamin Poulain]

Comment on attachment 231508 [details]
Patch

Looking into this in detail, I think the design is good.

Yesterday I thought we should unify solveDescendantBacktrackingActionForChild-solveAdjacentBacktrackingActionForDirectAdjacent and computeBacktrackingHeightFromDescendant-computeBacktrackingWidthFromIndirectAdjacent...but looking into this more I am less convinced.

We could templatize the functions to do that, but I think that would hurt readability. A little duplication is okay if the result is clearer.

I clear the review flag to remove the patch from the review queue until you add the tests.

[Yusuke Suzuki]

(In reply to comment #3)

Thank you for your review!

> (From update of attachment 231508 [details])
> Looking into this in detail, I think the design is good.
> 
> Yesterday I thought we should unify solveDescendantBacktrackingActionForChild-solveAdjacentBacktrackingActionForDirectAdjacent and computeBacktrackingHeightFromDescendant-computeBacktrackingWidthFromIndirectAdjacent...but looking into this more I am less convinced.
> 
> We could templatize the functions to do that, but I think that would hurt readability. A little duplication is okay if the result is clearer.

Right.
Personally, I think this duplication represents the implementation flow simple.
There's slightly different parts. For example, `computeBacktrackingHeightFromDescendant` has a special part to assign height information to adjacent fragments.

> 
> I clear the review flag to remove the patch from the review queue until you add the tests.

I'll add test cases!

[Yusuke Suzuki]

Created attachment 231904 [details]
Patch

[Build Bot]

Comment on attachment 231904 [details]
Patch

Attachment 231904 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5260851703447552

New failing tests:
media/W3C/video/readyState/readyState_during_canplay.html

[Build Bot]

Created attachment 231919 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Benjamin Poulain]

Comment on attachment 231904 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231904&action=review

That's awwweeesssooooome.

Great work.
The bots are sick. You don't need to update the patch, I'll land it manually tomorrow.

> LayoutTests/fast/selectors/backtracking-adjacent-optimized.html:23
> +        <a class="ok"></a>  <!-- Mathced. -->

Typo here: Mathced.

> LayoutTests/fast/selectors/backtracking-adjacent-optimized.html:24
> +        <a class="ng"></a>  <!-- Failed. And backtrack from here. -->

I would duplicate this a dozen of times. That we we can to find bugs in which we would exhaust the register allocator.

[Benjamin Poulain]

Created attachment 232008 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 232008 [details]
Patch for landing

Clearing flags on attachment: 232008

Committed r169298: <http://trac.webkit.org/changeset/169298>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.