Bug 131602 - CSS JIT: compile the :nth-child() pseudo class
Summary: CSS JIT: compile the :nth-child() pseudo class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-04-13 19:12 PDT by Benjamin Poulain
Modified: 2014-04-14 01:43 PDT (History)
16 users (show)

See Also:


Attachments
Patch (45.74 KB, patch)
2014-04-13 19:27 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (469.20 KB, application/zip)
2014-04-13 20:43 PDT, Build Bot
no flags Details
Patch (46.94 KB, patch)
2014-04-13 21:09 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2014-04-13 19:12:38 PDT
CSS JIT: compile the :nth-child() pseudo class
Comment 1 Benjamin Poulain 2014-04-13 19:27:02 PDT
Created attachment 229256 [details]
Patch
Comment 2 Build Bot 2014-04-13 20:43:46 PDT
Comment on attachment 229256 [details]
Patch

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

New failing tests:
http/tests/security/video-poster-cross-origin-crash.html
Comment 3 Build Bot 2014-04-13 20:43:50 PDT
Created attachment 229257 [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
Comment 4 Benjamin Poulain 2014-04-13 21:09:30 PDT
Created attachment 229258 [details]
Patch
Comment 5 Andreas Kling 2014-04-13 22:19:22 PDT
Comment on attachment 229258 [details]
Patch

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

> Source/WebCore/cssjit/SelectorCompiler.cpp:1025
> +        failureCases.append(m_assembler.branchTest32(Assembler::NonZero, inputDividend, Assembler::TrustedImm32(1)));

Could this be even tighter with branchTest8()?

> Source/WebCore/cssjit/SelectorCompiler.cpp:2016
> +                failureCases.append(m_assembler.branchTest32(Assembler::Zero, elementCounter, Assembler::TrustedImm32(1)));

Could this be even tighter with branchTest8()?
Comment 6 Andreas Kling 2014-04-13 22:19:26 PDT
Comment on attachment 229258 [details]
Patch

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

> Source/WebCore/cssjit/SelectorCompiler.cpp:1025
> +        failureCases.append(m_assembler.branchTest32(Assembler::NonZero, inputDividend, Assembler::TrustedImm32(1)));

Could this be even tighter with branchTest8()?

> Source/WebCore/cssjit/SelectorCompiler.cpp:2016
> +                failureCases.append(m_assembler.branchTest32(Assembler::Zero, elementCounter, Assembler::TrustedImm32(1)));

Could this be even tighter with branchTest8()?
Comment 7 Andreas Kling 2014-04-13 22:20:21 PDT
I wonder if we could shuffle the Node flags so the ones CSS JIT cares about are testable with 8-bit tests. (I also wonder if that would help anything.)
Comment 8 Benjamin Poulain 2014-04-13 23:22:35 PDT
(In reply to comment #7)
> I wonder if we could shuffle the Node flags so the ones CSS JIT cares about are testable with 8-bit tests. (I also wonder if that would help anything.)

If you look at test32, you'll notice that I already implemented that :)

Depending on the mask, it will select 8 bits instructions :)

Also, if you use Address with test32, it will be smart about changing the offset to perform a 8 bits operation :)
Comment 9 Andreas Kling 2014-04-13 23:29:50 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > I wonder if we could shuffle the Node flags so the ones CSS JIT cares about are testable with 8-bit tests. (I also wonder if that would help anything.)
> 
> If you look at test32, you'll notice that I already implemented that :)
> 
> Depending on the mask, it will select 8 bits instructions :)
> 
> Also, if you use Address with test32, it will be smart about changing the offset to perform a 8 bits operation :)

LOL okay! Good job already then :D
Comment 10 Andreas Kling 2014-04-14 01:12:15 PDT
Comment on attachment 229258 [details]
Patch

r=me.

We can definitely improve upon the modulo function if we decide that's useful, but this is totally fine for landing now!
Comment 11 Benjamin Poulain 2014-04-14 01:13:43 PDT
Comment on attachment 229258 [details]
Patch

(In reply to comment #10)
> (From update of attachment 229258 [details])
> r=me.
> 
> We can definitely improve upon the modulo function if we decide that's useful, but this is totally fine for landing now!

Thanks!
Comment 12 WebKit Commit Bot 2014-04-14 01:43:20 PDT
Comment on attachment 229258 [details]
Patch

Clearing flags on attachment: 229258

Committed r167218: <http://trac.webkit.org/changeset/167218>
Comment 13 WebKit Commit Bot 2014-04-14 01:43:27 PDT
All reviewed patches have been landed.  Closing bug.