Bug 74067 - Unify willMoveToNewOwnerDocument() and didMoveToNewOwnerDocument() for better performance
Summary: Unify willMoveToNewOwnerDocument() and didMoveToNewOwnerDocument() for better...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on:
Blocks: 59816
  Show dependency treegraph
 
Reported: 2011-12-08 01:54 PST by Hajime Morrita
Modified: 2011-12-25 23:05 PST (History)
8 users (show)

See Also:


Attachments
Patch (32.00 KB, patch)
2011-12-08 22:40 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (32.37 KB, patch)
2011-12-12 21:26 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (32.62 KB, patch)
2011-12-13 19:10 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (32.28 KB, patch)
2011-12-22 03:32 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (30.15 KB, patch)
2011-12-25 19:34 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (30.91 KB, patch)
2011-12-25 21:16 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch for landing (30.87 KB, patch)
2011-12-25 22:05 PST, 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 Hajime Morrita 2011-12-08 01:54:07 PST
This change unify
    virtual void Node::willMoveToNewOwnerDocument();
    virtual void Node::didMoveToNewOwnerDocument();
into
    virtual void Node::didMoveToNewOwnerDocument(Document* oldDocment, Document* newDocument);

This is preparation for Bug 59816.
This also comes with a potential performance win because we kill two virtual function call down to one.
Comment 1 Hajime Morrita 2011-12-08 22:40:38 PST
Created attachment 118533 [details]
Patch
Comment 2 Hajime Morrita 2011-12-08 22:42:05 PST
Hi Dimitri, could you take a look at this?
This and blocking Bug 59816 pushes forward to make TreeScope first class citizen.
Comment 3 WebKit Review Bot 2011-12-09 15:52:36 PST
Comment on attachment 118533 [details]
Patch

Attachment 118533 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10831400
Comment 4 Hajime Morrita 2011-12-11 19:00:52 PST
The redness on cr-linux is apparently due to tree breakage at that time.
Comment 5 Hajime Morrita 2011-12-12 21:26:54 PST
Created attachment 118948 [details]
Patch
Comment 6 Early Warning System Bot 2011-12-12 21:36:29 PST
Comment on attachment 118948 [details]
Patch

Attachment 118948 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10847234
Comment 7 Gustavo Noronha (kov) 2011-12-12 21:42:35 PST
Comment on attachment 118948 [details]
Patch

Attachment 118948 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10846218
Comment 8 Gyuyoung Kim 2011-12-12 21:43:58 PST
Comment on attachment 118948 [details]
Patch

Attachment 118948 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10845219
Comment 9 WebKit Review Bot 2011-12-12 21:46:47 PST
Comment on attachment 118948 [details]
Patch

Attachment 118948 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10845218
Comment 10 Hajime Morrita 2011-12-13 19:10:02 PST
Created attachment 119137 [details]
Patch
Comment 11 WebKit Review Bot 2011-12-13 20:06:14 PST
Comment on attachment 119137 [details]
Patch

Attachment 119137 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10874080
Comment 12 Hajime Morrita 2011-12-22 03:32:13 PST
Created attachment 120304 [details]
Patch
Comment 13 Gustavo Noronha (kov) 2011-12-22 03:43:36 PST
Comment on attachment 120304 [details]
Patch

Attachment 120304 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10998413
Comment 14 Gyuyoung Kim 2011-12-22 03:53:17 PST
Comment on attachment 120304 [details]
Patch

Attachment 120304 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10981480
Comment 15 Early Warning System Bot 2011-12-22 03:58:26 PST
Comment on attachment 120304 [details]
Patch

Attachment 120304 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10998418
Comment 16 WebKit Review Bot 2011-12-22 03:59:25 PST
Comment on attachment 120304 [details]
Patch

Attachment 120304 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10998416
Comment 17 Darin Adler 2011-12-22 08:45:51 PST
Comment on attachment 120304 [details]
Patch

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

The EWS bot is failing on the patch, so I think you’ll need a new version. The idea seems fine, but it can be done in a more-lightweight fashion.

> Source/WebCore/dom/Node.h:693
> +    virtual void didMoveToNewOwnerDocument(const NodeOwnerChange&);

It seems to me we should just pass the old document. The new document is easy to get, just call document(), so there is no need to pass it. Having a named structure just to pass two arguments also seems unnecessarily heavyweight.

> Source/WebCore/html/HTMLFormControlElement.h:123
> +    virtual void didMoveToNewOwnerDocument(const NodeOwnerChange&);

Please use OVERRIDE in all the didMoveToNewOwnerDocument function declarations. This will catch if we get the argument list wrong in any of them.
Comment 18 Hajime Morrita 2011-12-25 19:34:45 PST
Created attachment 120517 [details]
Patch
Comment 19 Hajime Morrita 2011-12-25 19:37:05 PST
Hi Darin, thanks you for taking look!
I updated the patch. Could you take another look?

> It seems to me we should just pass the old document. The new document is easy to get, just call document(), so there is no need to pass it. Having a named structure just to pass two arguments also seems unnecessarily heavyweight.
Right. The updated patch addresses this.

> 
> > Source/WebCore/html/HTMLFormControlElement.h:123
> > +    virtual void didMoveToNewOwnerDocument(const NodeOwnerChange&);
> 
> Please use OVERRIDE in all the didMoveToNewOwnerDocument function declarations. This will catch if we get the argument list wrong in any of them.
Fixed.
Comment 20 Darin Adler 2011-12-25 19:58:02 PST
Comment on attachment 120517 [details]
Patch

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

This is looking great. review- because of the HTMLInputElement being a bit too messy.

Also, one last idea. I think didMoveToNewDocument is a fine name for this. I don’t think we need to emphasize “owner document” just because the DOM does; the old functions had awkward names, but no reason that we should keep those awkward names.

> Source/WebCore/html/FormAssociatedElement.cpp:59
> +    if (element->fastHasAttribute(formAttr) && oldDocument)
> +        oldDocument->unregisterFormElementWithFormAttribute(this);

Probably should check oldDocument first for better performance in the new-element case.

> Source/WebCore/html/FormAssociatedElement.h:67
> +    void didMoveToNewOwnerDocument(Document* oldDocument) OVERRIDE;

This is not an override of a virtual function; I would not expect this to compile at all!

> Source/WebCore/html/HTMLInputElement.cpp:1510
> +        if (needsSuspensionCallback())

Since this function always calls needsSuspensionCallback at least once, it would be better if it called it only once and stored it in a local variable.

> Source/WebCore/html/HTMLMediaElement.cpp:3401
> +void HTMLMediaElement::setShouldDelayLoadEvent(Document* doc, bool shouldDelay)

Please don’t use the abbreviation “doc” in an argument name. The name document would be better.

This new function is messy. It’s not good for the media element itself to change its m_shouldDelayLoadEvent state when moving from one document to another. Instead, only the count should be updated when the element moves from document to the other and this function shouldn’t be called at all. I don’t even think didMoveToNewOwnerDocument should look at m_readyState. It should only look at m_shouldDelayLoadEvent.
Comment 21 Hajime Morrita 2011-12-25 21:16:11 PST
Created attachment 120524 [details]
Patch
Comment 22 Darin Adler 2011-12-25 21:28:09 PST
Comment on attachment 120524 [details]
Patch

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

Please fix the one thing I mentioned; otherwise this is ready to go.

> Source/WebCore/html/HTMLMediaElement.cpp:288
> +    if (m_readyState < HAVE_CURRENT_DATA && m_shouldDelayLoadEvent) {

We do not want to check m_readyState < HAVE_CURRENT_DATA here. If this somehow gets out of sync with m_shouldDelayLoadEvent it could result in us leaving a document with the wrong load event delay count. This logic should solely be based on the value of m_shouldDelayLoadEvent. The old code needed to check m_readyState because of a flaw in how the old code worked that is now fixed by your new function.
Comment 23 Hajime Morrita 2011-12-25 22:03:12 PST
Hi Darin, thanks for lighting review! I'll land this after addressing your point.

(In reply to comment #22)
> (From update of attachment 120524 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120524&action=review
> 
> Please fix the one thing I mentioned; otherwise this is ready to go.
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:288
> > +    if (m_readyState < HAVE_CURRENT_DATA && m_shouldDelayLoadEvent) {
> 
> We do not want to check m_readyState < HAVE_CURRENT_DATA here. If this somehow gets out of sync with m_shouldDelayLoadEvent it could result in us leaving a document with the wrong load event delay count. This logic should solely be based on the value of m_shouldDelayLoadEvent. The old code needed to check m_readyState because of a flaw in how the old code worked that is now fixed by your new function.
Comment 24 Hajime Morrita 2011-12-25 22:05:59 PST
Created attachment 120526 [details]
Patch for landing
Comment 25 Darin Adler 2011-12-25 22:26:39 PST
Comment on attachment 120526 [details]
Patch for landing

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

> Source/WebCore/dom/Node.cpp:2511
> +#else
> +    UNUSED_PARAM(oldDocument);

Hmm, I just noticed that oldDocument is never used on either side of the if statement. That means that you should leave out the argument name altogether. Without that I think we’ll fail to compile when MUTATION_OBSERVERS is enabled.

> Source/WebCore/dom/StyledElement.cpp:185
> +    // Normally we would be concerned about reseting the parent of those declarations in StyledElement::didMoveToNewDocument().

I think it’s spelled resetting rather than reseting.
Comment 26 WebKit Review Bot 2011-12-25 23:05:16 PST
Comment on attachment 120526 [details]
Patch for landing

Clearing flags on attachment: 120526

Committed r103679: <http://trac.webkit.org/changeset/103679>
Comment 27 WebKit Review Bot 2011-12-25 23:05:23 PST
All reviewed patches have been landed.  Closing bug.