Bug 90367 - [Chromium] ContextFeaturesClient::isEnabled is slow
Summary: [Chromium] ContextFeaturesClient::isEnabled is slow
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-02 02:05 PDT by Hayato Ito
Modified: 2012-07-09 01:54 PDT (History)
8 users (show)

See Also:


Attachments
Patch (6.96 KB, patch)
2012-07-03 00:18 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (6.97 KB, patch)
2012-07-03 00:22 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (7.07 KB, patch)
2012-07-08 23:37 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (7.09 KB, patch)
2012-07-08 23:41 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (9.42 KB, patch)
2012-07-09 00:14 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch for landing (9.37 KB, patch)
2012-07-09 00:50 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hayato Ito 2012-07-02 02:05:51 PDT
ContextFeatures::shadowDOMEnabled(), which is not a light operation, is called frequently in editing, such as in Position::findParent().
That causes unresponsiveness if some conditions are met.

Chromium side bug:
http://code.google.com/p/chromium/issues/detail?id=134398
Comment 1 Hayato Ito 2012-07-02 03:50:22 PDT
Morrita-san might be working on this.  I think we can avoid a high cost operation by using a cache in a layer somewhere.
Comment 2 Ryosuke Niwa 2012-07-02 09:47:21 PDT
Which function is calling this?
Comment 3 Hajime Morrita 2012-07-02 18:13:17 PDT
(In reply to comment #2)
> Which function is calling this?
from some shadow-specific conditionals.
I'm working on this btw.
Comment 4 Hajime Morrita 2012-07-02 21:02:01 PDT
Renaming the bug: The problem is that the function is slow, not being called frequently.
Comment 5 Hajime Morrita 2012-07-03 00:18:54 PDT
Created attachment 150549 [details]
Patch
Comment 6 Hajime Morrita 2012-07-03 00:20:02 PDT
Kent-san, could you take a look?
The original code was reviewed by Dimitri. But he's in vacation.
Comment 7 Hajime Morrita 2012-07-03 00:22:09 PDT
Created attachment 150551 [details]
Patch
Comment 8 Kent Tamura 2012-07-08 21:41:34 PDT
Comment on attachment 150551 [details]
Patch

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

> Source/WebCore/dom/ContextFeatures.h:46
> +        FeatureTypeSize

Needs a comment for this like "This should be at the last" or something.

> Source/WebKit/chromium/src/ContextFeaturesClientImpl.cpp:59
> +        bool isEnabled() const { return m_value == IsEnabled; }

Should have ASSERT(m_value != IsNotValid);

> Source/WebKit/chromium/src/ContextFeaturesClientImpl.cpp:70
> +        bool isValid(bool defaultValue) const
> +        {
> +            return m_value != IsNotValid && m_defaultValue == defaultValue;
> +        }

The function name looks wrong because this function checks not only IsNotValid but also m_defaultValue.
This should be "needsToRefreshValue()" or something.

> Source/WebKit/chromium/src/ContextFeaturesClientImpl.cpp:74
> +        bool m_defaultValue; // Needs to be traked as a part of the signature since it can be changed dynamically.

Do we already have such case?

> Source/WebKit/chromium/src/ContextFeaturesClientImpl.cpp:88
> +    void validateAgainst(Document* document)
> +    {
> +        String currentDomain = document->securityOrigin()->domain();
> +        if (currentDomain == m_domain)
> +            return;
> +        m_domain = currentDomain;
> +        for (size_t i = 0; i < ContextFeatures::FeatureTypeSize; ++i)
> +            m_entries[i] = Entry();
> +    }

I prefer defining this function at outside of the class definition because this function is not small.

> Source/WebKit/chromium/src/ContextFeaturesClientImpl.cpp:90
> +    Entry* at(ContextFeatures::FeatureType type)

* "at()" sounds strange.  entryFor()?

* You can make the return type "Entry&".

> Source/WebKit/chromium/src/ContextFeaturesClientImpl.cpp:116
> +    cache->validateAgainst(document);

Why we need to call validateAgainst() every time?
Comment 9 Hajime Morrita 2012-07-08 23:37:25 PDT
Created attachment 151190 [details]
Patch
Comment 10 Hajime Morrita 2012-07-08 23:41:37 PDT
Created attachment 151191 [details]
Patch
Comment 11 Hajime Morrita 2012-07-09 00:14:55 PDT
Created attachment 151196 [details]
Patch
Comment 12 Hajime Morrita 2012-07-09 00:22:13 PDT
Comment on attachment 150551 [details]
Patch

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

>> Source/WebCore/dom/ContextFeatures.h:46
>> +        FeatureTypeSize
> 
> Needs a comment for this like "This should be at the last" or something.

Done.

>> Source/WebKit/chromium/src/ContextFeaturesClientImpl.cpp:59
>> +        bool isEnabled() const { return m_value == IsEnabled; }
> 
> Should have ASSERT(m_value != IsNotValid);

Fixed.

>> Source/WebKit/chromium/src/ContextFeaturesClientImpl.cpp:70
>> +        }
> 
> The function name looks wrong because this function checks not only IsNotValid but also m_defaultValue.
> This should be "needsToRefreshValue()" or something.

Ok, renamed.

>> Source/WebKit/chromium/src/ContextFeaturesClientImpl.cpp:74
>> +        bool m_defaultValue; // Needs to be traked as a part of the signature since it can be changed dynamically.
> 
> Do we already have such case?

Yes. some tests change default values (RutimeEnabledFeatures flags) during the test, which needs a logic like this.

>> Source/WebKit/chromium/src/ContextFeaturesClientImpl.cpp:88
>> +    }
> 
> I prefer defining this function at outside of the class definition because this function is not small.

Done.

>> Source/WebKit/chromium/src/ContextFeaturesClientImpl.cpp:90
>> +    Entry* at(ContextFeatures::FeatureType type)
> 
> * "at()" sounds strange.  entryFor()?
> 
> * You can make the return type "Entry&".

Done.

>> Source/WebKit/chromium/src/ContextFeaturesClientImpl.cpp:116
>> +    cache->validateAgainst(document);
> 
> Why we need to call validateAgainst() every time?

Good question. 
I added ContextFeatureClient::urlDidChange() which is called from Document::setURL(), and did the validation there.
Comment 13 Kent Tamura 2012-07-09 00:25:54 PDT
Comment on attachment 151196 [details]
Patch

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

> Source/WebCore/dom/ContextFeatures.h:59
> +    void urlDidChange(Document*);
>  private:

nit: need a blank line before private:

> Source/WebKit/chromium/src/ContextFeaturesClientImpl.cpp:40
> +using namespace WTF;

Is this necessary?
Comment 14 Hajime Morrita 2012-07-09 00:50:27 PDT
Created attachment 151204 [details]
Patch for landing
Comment 15 Hajime Morrita 2012-07-09 00:52:23 PDT
Kent-san, thanks for the quick round.
I addressed the remaining points and am landing the patch.

(In reply to comment #13)
> (From update of attachment 151196 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151196&action=review
> 
> > Source/WebCore/dom/ContextFeatures.h:59
> > +    void urlDidChange(Document*);
> >  private:
> 
> nit: need a blank line before private:
> 
> > Source/WebKit/chromium/src/ContextFeaturesClientImpl.cpp:40
> > +using namespace WTF;
> 
> Is this necessary?
Comment 16 WebKit Review Bot 2012-07-09 01:54:06 PDT
Comment on attachment 151204 [details]
Patch for landing

Clearing flags on attachment: 151204

Committed r122099: <http://trac.webkit.org/changeset/122099>
Comment 17 WebKit Review Bot 2012-07-09 01:54:17 PDT
All reviewed patches have been landed.  Closing bug.