Bug 90367 – [Chromium] ContextFeaturesClient::isEnabled is slow

• Component: HTML Editing
• Status: RESOLVED FIXED

[Hayato Ito]

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

[Hayato Ito]

Morrita-san might be working on this.  I think we can avoid a high cost operation by using a cache in a layer somewhere.

[Ryosuke Niwa]

Which function is calling this?

[Hajime Morrita]

(In reply to comment #2)
> Which function is calling this?
from some shadow-specific conditionals.
I'm working on this btw.

[Hajime Morrita]

Renaming the bug: The problem is that the function is slow, not being called frequently.

[Hajime Morrita]

Created attachment 150549 [details]
Patch

[Hajime Morrita]

Kent-san, could you take a look?
The original code was reviewed by Dimitri. But he's in vacation.

[Hajime Morrita]

Created attachment 150551 [details]
Patch

[Kent Tamura]

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?

[Hajime Morrita]

Created attachment 151190 [details]
Patch

[Hajime Morrita]

Created attachment 151191 [details]
Patch

[Hajime Morrita]

Created attachment 151196 [details]
Patch

[Hajime Morrita]

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.

[Kent Tamura]

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?

[Hajime Morrita]

Created attachment 151204 [details]
Patch for landing

[Hajime Morrita]

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?

[WebKit Review Bot]

Comment on attachment 151204 [details]
Patch for landing

Clearing flags on attachment: 151204

Committed r122099: <http://trac.webkit.org/changeset/122099>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.