Bug 34029 - REGRESSION (r47444): PLT is 1% slower due to implementation of :valid and :invalid CSS selectors
Summary: REGRESSION (r47444): PLT is 1% slower due to implementation of :valid and :in...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2010-01-22 18:07 PST by Adele Peterson
Modified: 2010-01-26 17:36 PST (History)
1 user (show)

See Also:


Attachments
preliminary patch (12.25 KB, patch)
2010-01-22 18:07 PST, Adele Peterson
no flags Details | Formatted Diff | Diff
preliminary patch (15.60 KB, patch)
2010-01-22 18:11 PST, Adele Peterson
no flags Details | Formatted Diff | Diff
preliminary patch (16.17 KB, patch)
2010-01-22 18:24 PST, Adele Peterson
no flags Details | Formatted Diff | Diff
patch (4.20 KB, patch)
2010-01-25 11:28 PST, Adele Peterson
darin: review-
Details | Formatted Diff | Diff
patch (7.11 KB, patch)
2010-01-26 12:43 PST, Adele Peterson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adele Peterson 2010-01-22 18:07:10 PST
Created attachment 47253 [details]
preliminary patch

:valid and :invalid CSS selectors slows down pages that make no use of the new feature

<rdar://problem/7169464>
Comment 1 Adele Peterson 2010-01-22 18:11:59 PST
Created attachment 47254 [details]
preliminary patch
Comment 2 Adele Peterson 2010-01-22 18:24:23 PST
Created attachment 47255 [details]
preliminary patch
Comment 3 Adele Peterson 2010-01-23 08:22:12 PST
ignore the patches above- this approach doesn't save much because of how common maxLength is
Comment 4 Adele Peterson 2010-01-25 11:28:41 PST
Created attachment 47360 [details]
patch

I'm still running tests, but I'm posting this for review anyway.  I set a breakpoint and verified with this patch, I won't hit the validity checks at all while running the PLT.  The tests I'm constructing now involve adding and removing valid/invalid pseudostyles and making sure that style sharing happens in the right circumstances.
Comment 5 Adele Peterson 2010-01-25 11:29:09 PST
Comment on attachment 47360 [details]
patch

Also, considerValidity is not a great name - better ideas?
Comment 6 Darin Adler 2010-01-25 11:34:27 PST
Comment on attachment 47360 [details]
patch

> +                if (!m_element->document()->considerValidity())

For boolean member functions we try to make ones that fit in a sentence "document <xxx>" so this function needs a different name, one that is not a verb phrase. Maybe "containsValidityStyleRules" or reverse the sense and have it be "containsNoValidityStyleRules" since there *may* be some rules, but there may no longer be. I hope there's a good, relatively short name. I chose the word "rules" over "selectors" in my name because of length. I'm not sure "contains" is the right word.

> +            case CSSSelector::PseudoValid: {
> +                if (!e)
> +                    return false;
> +                e->document()->setConsiderValidity();
> +                return e->willValidate() && e->isValidFormControlElement();
> +            } case CSSSelector::PseudoInvalid: {
> +                if (!e)
> +                    return false;
> +                e->document()->setConsiderValidity();
> +                return e->willValidate() && !e->isValidFormControlElement();
> +            } case CSSSelector::PseudoChecked: {

No braces needed here. It's also strange to put the end brace on the same line as the case. Not our usual style, I don't think.

review- because of the lack of a test case
Comment 7 Adele Peterson 2010-01-25 11:52:44 PST
> > +            case CSSSelector::PseudoValid: {
> > +                if (!e)
> > +                    return false;
> > +                e->document()->setConsiderValidity();
> > +                return e->willValidate() && e->isValidFormControlElement();
> > +            } case CSSSelector::PseudoInvalid: {
> > +                if (!e)
> > +                    return false;
> > +                e->document()->setConsiderValidity();
> > +                return e->willValidate() && !e->isValidFormControlElement();
> > +            } case CSSSelector::PseudoChecked: {
> 
> No braces needed here. It's also strange to put the end brace on the same line
> as the case. Not our usual style, I don't think.

That's the style used in the rest of the method for cases with multiple lines.
Comment 8 Adele Peterson 2010-01-26 12:43:07 PST
Created attachment 47434 [details]
patch
Comment 9 Adele Peterson 2010-01-26 17:36:07 PST
Committed revision 53878.