Bug 137189 - Try to detect no-op innerHTML mutations and avoid repainting.
Summary: Try to detect no-op innerHTML mutations and avoid repainting.
Status: RESOLVED LATER
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords: InRadar, Performance
Depends on:
Blocks:
 
Reported: 2014-09-27 23:57 PDT by Andreas Kling
Modified: 2022-08-12 22:07 PDT (History)
9 users (show)

See Also:


Attachments
Patch (5.16 KB, patch)
2014-09-28 00:11 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2014-09-27 23:57:05 PDT
We should try to avoid repainting when setting an element's innerHTML a HTML string that's equivalent to the existing DOM subtree.

<rdar://problem/18241048>
Comment 1 Andreas Kling 2014-09-28 00:11:12 PDT
Created attachment 238805 [details]
Patch

Let's see what EWS thinks of this..
Comment 2 Alexey Proskuryakov 2014-09-28 00:39:00 PDT
A node that does not have a JS wrapper could still be accessible from JS, e.g. if it's part of editing undo stack, XPath node set, and there are more cases like that. So there are many ways in which this change is observable,  

I think that something very similar has been tried before, but I do not know how to find when. Perhaps it was part of Acid3 performance work? Darin may remember better.
Comment 3 Andreas Kling 2014-09-28 09:46:03 PDT
(In reply to comment #2)
> A node that does not have a JS wrapper could still be accessible from JS, e.g. if it's part of editing undo stack, XPath node set, and there are more cases like that. So there are many ways in which this change is observable,  

Perhaps we could use something like hasOneRef()?

> 
> I think that something very similar has been tried before, but I do not know how to find when. Perhaps it was part of Acid3 performance work? Darin may remember better.

This was attempted before, for all tag names, but broke plugin loading IIRC. My approach is to have a small whitelist of simple tags.
Comment 4 Darin Adler 2014-09-28 18:16:35 PDT
(In reply to comment #2)
> A node that does not have a JS wrapper could still be accessible from JS, e.g. if it's part of editing undo stack, XPath node set, and there are more cases like that. So there are many ways in which this change is observable.

I’d like to understand this better. I agree that a node can be accessible from JavaScript even if it doesn’t have a wrapper. But how would the lack of change in the identity of a node be observable if the node has no properties and does not appear as a property of any JavaScript object (including a collection)?
Comment 5 Alexey Proskuryakov 2014-09-29 00:41:05 PDT
I think that the difference is observable in a scenario like:

- we have a node accessible from JavaScript;
- JS code performs an assignment to innerHTML that we optimize away;
- then JS code accesses the node, and finds one that is in document tree, instead of one outside the tree.

In the case of XPath, document mutation should invalidate result sets, but as we don't do the mutation, the sets remain intact.
Comment 6 Andreas Kling 2014-10-01 16:25:09 PDT
It's worth noting that we already short-circuit innerHTML mutations if they are found to only affect the content of a single text node:

    if (hasOneTextChild(&containerNode.get()) && hasOneTextChild(fragment.get())) {
        downcast<Text>(*containerNode->firstChild()).setData(downcast<Text>(*fragment->firstChild()).data(), ec); 
        return;
    }

I wonder if there's really any harm in keeping the old DOM subtree alive, even if it's observable from JS.
Comment 7 Ahmad Saleem 2022-08-12 10:57:34 PDT
Patch does not have any test case but I can see this has not landed in Webkit Github and the source does not contain code from the patch:

https://github.com/WebKit/WebKit/blob/0eaa48964a00d47f5b1dd9d5fd8b589d5ac74b13/Source/WebCore/editing/markup.cpp

rniwa@webkit.org - Is it something needed now or this can be marked as "RESOLVED LATER"? Would appreciate your input?
Comment 8 Ryosuke Niwa 2022-08-12 22:07:22 PDT
Yeah, -> Later.