Bug 213659 – [iOS] Editable regions causes ~1% slowdown in PLT5

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Daniel Bates]

There is a bit of cost to compute the editable region that shows up as a ~1% slowdown in PLT5.

[Daniel Bates]

<rdar://problem/64361390>

[Daniel Bates]

Created attachment 402909 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 402909 [details]
Patch

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

> Source/WebCore/page/Page.cpp:933
> +        ASSERT(renderView->layer()); // Main frame render view always has a layer.
> +        renderView->layer()->invalidateEventRegion(RenderLayer::EventRegionInvalidationReason::SettingDidChange);

Just invalidating the event region in the root layer seems wrong. We need to invalidate it on all layers.

> Source/WebCore/page/SettingsBase.cpp:322
> +    if (m_page)
> +        m_page->setEditableRegionEnabled(m_page->settings().visibleDebugOverlayRegions() & EditableElementRegion);

Why is visibleDebugOverlayRegionsChanged calling setEditableRegionEnabled ?

[Daniel Bates]

Comment on attachment 402909 [details]
Patch

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

>> Source/WebCore/page/Page.cpp:933
>> +        renderView->layer()->invalidateEventRegion(RenderLayer::EventRegionInvalidationReason::SettingDidChange);
> 
> Just invalidating the event region in the root layer seems wrong. We need to invalidate it on all layers.

of course, that's what I want to do....I assumed just touching the main frame would invalidate all other layers, but I didn't check...

[Daniel Bates]

Created attachment 402929 [details]
Patch

[Daniel Bates]

Comment on attachment 402929 [details]
Patch

Got to invalidate all layers...

[Daniel Bates]

Created attachment 402967 [details]
Patch

[Daniel Bates]

Created attachment 402971 [details]
Patch

[Darin Adler]

Comment on attachment 402971 [details]
Patch

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

I am confused about the lifetime of this setting. If WebCore::Page makes the setting persist forever, then why does the UI process keep setting state back to NO? What sets it to true on a new page if the web process crashes?

Generally a bit puzzled.

General idea seems fine, but the details seem to not be 100% solid. What is the invariant here about how this gets set? When it does get set, what guarantees everything is regenerated? How long does the flag last once it’s true? What can set it back to false?

> Source/WebCore/page/Page.cpp:930
> +    auto frameView = makeRefPtr(mainFrame().view());

Seems strange that we are putting FrameView into a local variable if we are not null checking it.

Also not sure why doing a ref/deref on it is particularly valuable here.

> Source/WebCore/page/SettingsBase.cpp:322
> +        m_page->setEditableRegionEnabled(m_page->settings().visibleDebugOverlayRegions() & EditableElementRegion);

This seems wrong. We don’t want to set this back to false if it was set to true by _requestTextInputContextsInRect just because the debug overlay regions switch was flipped.

> Source/WebCore/rendering/EventRegion.h:90
> +    void ensureEditableRegion() { m_editableRegion = Region { }; }

This implementation does not match the name. The name implies this function will leave the editable region alone unless it doesn’t exist at all, in which case it will create one. But the implementation will overwrite an existing editable region with an empty one.

Is that what the caller wants? Can we name this more appropriately?

> Source/WebCore/rendering/RenderBlock.cpp:1267
> -        if (!isTextControl())
> +        if (page().isEditableRegionEnabled() && !isTextControl())
>              needsTraverseDescendants |= document().mayHaveEditableElements();

Must admit I don’t understand this one.

> Source/WebCore/rendering/RenderElement.cpp:749
> +            if (page().isEditableRegionEnabled()) {

Maybe we can do the page() checking after the userModify checking? Should be decided based on which is faster. Do the faster check first.

> Source/WebCore/rendering/RenderLayerBacking.cpp:1711
> -    if (renderer().document().mayHaveEditableElements())
> +    if (renderer().page().isEditableRegionEnabled() && renderer().document().mayHaveEditableElements())

Are these correctly ordered with the faster check first?

> Source/WebCore/rendering/RenderLayerBacking.cpp:1743
> +        // The existence of the editable region, even if the document has 0 editable elements, is the
> +        // performance optimization. It allows the UI process to avoid messaging the web process to
> +        // ask for editable elements when there are none. If there is no editable region then the UI
> +        // process must always message the web process because it does not know whether or not the
> +        // page has editable elements.

This long comment still doesn’t explain to me why the code below is correct or needed. Maybe a shorter comment that focuses on why this code is beneficial rather than trying to thoroughly describe the background.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2361
> +inline void RenderLayerCompositor::applyToCompositedLayerIncludingDescendants(RenderLayer& layer, const ApplyFunctionType& function)

The "inline" here is not needed. Won’t even make it more likely that it gets inlined. Please omit.

> Source/WebCore/testing/InternalSettings.cpp:290
> +    m_page->setEditableRegionEnabled(true);

Seems an unfortunate default if it makes all tests slightly slower and also tests the less common mode.

> Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:123
> +    bool seenAnEditableRegion = false;

Grammatically questionable to name this "seen". Maybe "saw"?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:938
> +    _editableRegionEnabled = NO;

I don’t understand why this is helpful. Why do we want to forget whether we have told the page to enable editable regions?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4303
> +    _editableRegionEnabled = NO;

Ditto.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:5232
> +    if (!_editableRegionEnabled)

Is this if statement an important optimization? I suggest omitting the "if".

[Daniel Bates]

Comment on attachment 402971 [details]
Patch

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

>> Source/WebCore/page/Page.cpp:930
>> +    auto frameView = makeRefPtr(mainFrame().view());
> 
> Seems strange that we are putting FrameView into a local variable if we are not null checking it.
> 
> Also not sure why doing a ref/deref on it is particularly valuable here.

yeah, not needed or useful.

>> Source/WebCore/page/SettingsBase.cpp:322
>> +        m_page->setEditableRegionEnabled(m_page->settings().visibleDebugOverlayRegions() & EditableElementRegion);
> 
> This seems wrong. We don’t want to set this back to false if it was set to true by _requestTextInputContextsInRect just because the debug overlay regions switch was flipped.

yep, this is wrong..oops

>> Source/WebCore/rendering/EventRegion.h:90
>> +    void ensureEditableRegion() { m_editableRegion = Region { }; }
> 
> This implementation does not match the name. The name implies this function will leave the editable region alone unless it doesn’t exist at all, in which case it will create one. But the implementation will overwrite an existing editable region with an empty one.
> 
> Is that what the caller wants? Can we name this more appropriately?

yep, this is a foot-gun. I'll fix it up to actually ensure not overwrite.

>> Source/WebCore/rendering/RenderBlock.cpp:1267
>>              needsTraverseDescendants |= document().mayHaveEditableElements();
> 
> Must admit I don’t understand this one.

This prevents (1) in the WebCore ChangeLog.

>> Source/WebCore/rendering/RenderElement.cpp:749
>> +            if (page().isEditableRegionEnabled()) {
> 
> Maybe we can do the page() checking after the userModify checking? Should be decided based on which is faster. Do the faster check first.

I think this is the faster check and it will take me 4-8 hours for PLT5 to run to know for sure though maybe micro-benchmarking will be enough 🤷‍♂️.

>> Source/WebCore/rendering/RenderLayerBacking.cpp:1711
>> +    if (renderer().page().isEditableRegionEnabled() && renderer().document().mayHaveEditableElements())
> 
> Are these correctly ordered with the faster check first?

I think so. 🤷‍♂️I actually had them in the opposite order in earlier iterations, but it takes 4-8 hours to run PLT5 and I got good enough results with this patch. Editable region will ONLY be used on iPad and only when the requestTextInputContext IPI is invoked. So, I figured that's a smaller audience than all iOS devices.

>> Source/WebCore/rendering/RenderLayerBacking.cpp:1743
>> +        // page has editable elements.
> 
> This long comment still doesn’t explain to me why the code below is correct or needed. Maybe a shorter comment that focuses on why this code is beneficial rather than trying to thoroughly describe the background.

Then maybe no comment is needed. I'll just remove.

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:2361
>> +inline void RenderLayerCompositor::applyToCompositedLayerIncludingDescendants(RenderLayer& layer, const ApplyFunctionType& function)
> 
> The "inline" here is not needed. Won’t even make it more likely that it gets inlined. Please omit.

OK.

>> Source/WebCore/testing/InternalSettings.cpp:290
>> +    m_page->setEditableRegionEnabled(true);
> 
> Seems an unfortunate default if it makes all tests slightly slower and also tests the less common mode.

I'll think about flipping it.

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:938
>> +    _editableRegionEnabled = NO;
> 
> I don’t understand why this is helpful. Why do we want to forget whether we have told the page to enable editable regions?

I put it here because this function is called if the web process crashes via processDidExit. When that happens then UI process needs to know it has to tell the web process to activate the future again. I could move it to processDidExit, but I thought it fit with the clean up interaction concept more generally. So, I put it here.

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:5232
>> +    if (!_editableRegionEnabled)
> 
> Is this if statement an important optimization? I suggest omitting the "if".

haha, I actually added this if statement just for you! Because you've called me out in other patches for invoking a function that I know a priori would early return. Now you got me curious, when should you check before calling a function and when shouldn't you. Maybe I'm misremembering, but could the deciding factor be whether that function performs an early bail out?

[Daniel Bates]

BTW thanks for the review Darin.

[Daniel Bates]

Comment on attachment 402971 [details]
Patch

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

>>> Source/WebCore/page/Page.cpp:930
>>> +    auto frameView = makeRefPtr(mainFrame().view());
>> 
>> Seems strange that we are putting FrameView into a local variable if we are not null checking it.
>> 
>> Also not sure why doing a ref/deref on it is particularly valuable here.
> 
> yeah, not needed or useful.

Actually I'll null check it

[Daniel Bates]

Comment on attachment 402971 [details]
Patch

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

>> Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:123
>> +    bool seenAnEditableRegion = false;
> 
> Grammatically questionable to name this "seen". Maybe "saw"?

OK

[Daniel Bates]

Created attachment 403015 [details]
Patch

[Daniel Bates]

Just for me: If the web process crashes maybe the ui process should tell the web process to enable editable region if it was previously enabled. It would make a -request a bit faster.

[Daniel Bates]

Comment on attachment 403015 [details]
Patch

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

> Source/WebCore/page/SettingsBase.cpp:324
> +#if ENABLE(EDITABLE_REGION)
> +    if (m_page && !m_page->isEditableRegionEnabled() && m_page->settings().visibleDebugOverlayRegions() & EditableElementRegion)
> +        m_page->setEditableRegionEnabled(true);
> +#endif
> +}

r-; this is not good enough: the setting will not ensure editable regions are still enabled between page loads. I may just remove this change handler....

[Daniel Bates]

Created attachment 403018 [details]
Patch

[Daniel Bates]

Created attachment 403021 [details]
Patch

[Daniel Bates]

Comment on attachment 403021 [details]
Patch

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

> Source/WebCore/page/Page.cpp:934
> +    if (m_isEditableRegionEnabled == enabled)
> +        return;
> +    m_isEditableRegionEnabled = enabled;
> +    auto frameView = makeRefPtr(mainFrame().view());
> +    if (!frameView)
> +        return;
> +    if (auto* renderView = frameView->renderView())
> +        renderView->compositor().invalidateEventRegionForAllLayers();

This should be pushed to FrameView or RenderView because a Page sticks around and is repurposed.

[Daniel Bates]

Comment on attachment 403021 [details]
Patch

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

>> Source/WebCore/page/Page.cpp:934
>> +        renderView->compositor().invalidateEventRegionForAllLayers();
> 
> This should be pushed to FrameView or RenderView because a Page sticks around and is repurposed.

Probably ^^^ are still not the right place. Just need a bit per top-level document.

[Daniel Bates]

Comment on attachment 403021 [details]
Patch

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

>>> Source/WebCore/page/Page.cpp:934
>>> +        renderView->compositor().invalidateEventRegionForAllLayers();
>> 
>> This should be pushed to FrameView or RenderView because a Page sticks around and is repurposed.
> 
> Probably ^^^ are still not the right place. Just need a bit per top-level document.

oh, I know what happened here. I was wavering between making this a forever setting triggered by UI process or making this setting stick only for the current page. Basically the idea behind the former is that once a user uses the feature I'm betting they will use it across pages forever after. The later is more pessimistic. I ultimately decided on the later and forgot to stage my change to Page::didFinishLoad() to clear out m_isEditableRegionEnabled....

[Daniel Bates]

Created attachment 403118 [details]
Patch

Simplified the approach: no messaging to enable editable region from UI process. Now editable region will only be built if either WebPage::textInputContextsInRect() is invoked for the current page  or editable element region debug overlay is enabled (or both). FYI WebPage::textInputContextsInRect() is what is called by -_requestTextInputContextsInRect in the UI process. Because legacy WebKit does not use event region (and hence does not use editable region) I put the logic WebPage::textInputContextsInRect() instead of pushing it down into Page::editableElementsInRect(). I moved the checks for whether to build the editable region to be after checking if there are editable elements based on feedback from Darin though I do not know if improves perf

[Daniel Bates]

(In reply to Darin Adler from comment #9)
> Comment on attachment 402971 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=402971&action=review
> 
> I am confused about the lifetime of this setting. If WebCore::Page makes the
> setting persist forever, then why does the UI process keep setting state
> back to NO? 

Updated patch. UI process no longer maintains setting state.

> What sets it to true on a new page if the web process crashes?
> 

In new patch it is now set to true if either:
    1. Editable element region debug overlay is enabled
    Or
    2. -_requestTextInputContextsInRect IPI is invoked (more precisely when WebPage::textInputContextsInRect() is invoked, but only the IPI calls it)

When web process crashes then if it was enabled via (1) it will be automatically be re-enabled by virtue of the process reading its settings. Otherwise, it won't be and this gives up a tiny bit of performance for the first -_requestTextInputContextsInRect invocation in the new process, but think that's OK for now.

> Generally a bit puzzled.
> 
> General idea seems fine, but the details seem to not be 100% solid. What is
> the invariant here about how this gets set? When it does get set, what
> guarantees everything is regenerated? 

^^^ answers all of this

> How long does the flag last once it’s true? What can set it back to false?

If enabled via (1) then stays true until the setting is disabled. If enabled via (2) then until the next page is committed or the web processes crashes (whichever comes first).

[Daniel Bates]

Comment on attachment 403118 [details]
Patch

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

> Source/WebCore/page/Page.cpp:941
> +void Page::shouldBuildEditableRegion() const

This is wrong. Should return bool

[Daniel Bates]

Created attachment 403122 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 403122 [details]
Patch

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

> Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:142
> +    // If we didn't see an editable region then we don't know whether or not there are
> +    // editable elements in the rect. We only know that the web process did not do an
> +    // event region paint to find them. The caller will have to ask the web process to
> +    // know for sure.
> +    return !sawAnEditableRegion;

Rather than this long-winded comment, you could return an Optional<bool>

[Daniel Bates]

Comment on attachment 403122 [details]
Patch

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

Thanks for the review Simon

>> Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:142
>> +    return !sawAnEditableRegion;
> 
> Rather than this long-winded comment, you could return an Optional<bool>

OK

[Daniel Bates]

Comment on attachment 403122 [details]
Patch

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

>>> Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:142
>>> +    return !sawAnEditableRegion;
>> 
>> Rather than this long-winded comment, you could return an Optional<bool>
> 
> OK

Just for me: 0 callers today benefit from having a differentiation between Nullopt and false...

[Daniel Bates]

Comment on attachment 403122 [details]
Patch

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

> Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:123
> +    bool sawAnEditableRegion = false;

Actually, I'm not going to change the return type to Optional<>, but to make things a tiny bit clearer I'm going to change this to:

bool possiblyHasEditableElements = true;

> Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:136
> +        sawAnEditableRegion |= hasEditableRegion;

Then change this to:

if (hasEditableRegion)
    possiblyHasEditableElements = false;

>>>> Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:142
>>>> +    return !sawAnEditableRegion;
>>> 
>>> Rather than this long-winded comment, you could return an Optional<bool>
>> 
>> OK
> 
> Just for me: 0 callers today benefit from having a differentiation between Nullopt and false...

And lastly change this to:

return possiblyHasEditableElements;

I will delete the comment as well as I think the name of this local explains what the comment saids.

This function is named mayContain because it can return false positives - saids rect contains an editable element when it does not, but it never returns a false negative - if it saids the rects does not contain an editable element then it 100% doesn't.

[Daniel Bates]

Created attachment 403217 [details]
To Land

[Daniel Bates]

Comment on attachment 403217 [details]
To Land

Clearing flags on attachment: 403217

Committed r263762: <https://trac.webkit.org/changeset/263762>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.