Avoid manual ref/deref in AsyncScriptRunner by using PendingScript
Created attachment 77642 [details] Patch
Comment on attachment 77642 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77642&action=review I’m going to say review+ but I do wonder why the PendingScript changes here were needed at all. > WebCore/ChangeLog:23 > + * dom/PendingScript.cpp: > + (WebCore::PendingScript::PendingScript): > + * dom/PendingScript.h: No explanation of why you changed this class to no longer inline its constructors. It doesn’t seem required by the rest of the patch. And you didn’t touch the assignment operator. I’m a bit puzzled. Why? > WebCore/dom/AsyncScriptRunner.cpp:48 > for (size_t i = 0; i < m_scriptsToExecuteSoon.size(); ++i) { > - m_scriptsToExecuteSoon[i].first->element()->deref(); // Balances ref() in executeScriptSoon(). > m_document->decrementLoadEventDelayCount(); > } No braces needed now that this is a 1-line for loop body. > WebCore/dom/AsyncScriptRunner.cpp:61 > + PendingScript pendingScript; > + pendingScript.adoptElement(element); Should we make a suitable constructor. It seems it would be better to construct the PendingScript ready to go rather than empty-constructing it and then setting the two pieces separately. Also, the function name adoptElement is not good! This function doesn’t “adopt” an element. In WebKit we call a function “adopt” when it takes ownership of something in a unusual way, such as when a String takes over storage formerly owned by a Vector or a smart pointer takes a reference in the function adoptRef. > WebCore/dom/PendingScript.h:44 > class PendingScript : public CachedResourceClient { The dual nature of the PendingScript class is not great. We treat these as data objects that we can copy around, but they are also CachedResourceClient objects that can perform loading. It seems impossible to correctly copy one while its loading. > WebCore/dom/PendingScript.h:47 > + PendingScript(const PendingScript& other); No need for the argument name “other” here.
I would suggest running WebCore/benchmarks/parser/ before landing just to make sure.
Created attachment 77908 [details] Patch
> No explanation of why you changed this class to no longer inline its constructors. It doesn’t seem required by the rest of the patch. And you didn’t touch the assignment operator. I’m a bit puzzled. Why? Reverted. > No braces needed now that this is a 1-line for loop body. Done. > Should we make a suitable constructor. It seems it would be better to construct the PendingScript ready to go rather than empty-constructing it and then setting the two pieces separately. Agreed. Done. > Also, the function name adoptElement is not good! This function doesn’t “adopt” an element. In WebKit we call a function “adopt” when it takes ownership of something in a unusual way, such as when a String takes over storage formerly owned by a Vector or a smart pointer takes a reference in the function adoptRef. Renamed to setElement(). > The dual nature of the PendingScript class is not great. We treat these as data objects that we can copy around, but they are also CachedResourceClient objects that can perform loading. It seems impossible to correctly copy one while its loading. It is quite possible I am missing something, so please explain if I am. But the CachedResourceClient here is never actually used. It only exist to keep a reference so that the CachedResource's buffer isn't purged. This allows the owner of a PendingScript to not have to worry about the data being purged out from under it. > No need for the argument name “other” here. Moot now that it is inlined again. > I would suggest running WebCore/benchmarks/parser/ before landing just to make sure. I take it this is moot now that the inlining in PendingScript isn't changed?
Comment on attachment 77908 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77908&action=review > WebCore/dom/AsyncScriptRunner.cpp:87 > + CachedScript* cachedScript = scripts[i].cachedScript(); > + RefPtr<Element> element = scripts[i].releaseElementAndClear(); > + toScriptElement(element.get())->execute(cachedScript); releaseElementAndClear and ScriptElement::execute BOTH calls m_cachedScript->removeClient(). Are you sure we want to do this?
(In reply to comment #6) > (From update of attachment 77908 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=77908&action=review > > > WebCore/dom/AsyncScriptRunner.cpp:87 > > + CachedScript* cachedScript = scripts[i].cachedScript(); > > + RefPtr<Element> element = scripts[i].releaseElementAndClear(); > > + toScriptElement(element.get())->execute(cachedScript); > > releaseElementAndClear and ScriptElement::execute BOTH calls m_cachedScript->removeClient(). Are you sure we want to do this? Yes, they are different clients. releaseElementAndClear removes the PendingScript client and execute removes the ScriptElement client.
Comment on attachment 77908 [details] Patch (In reply to comment #7) > > releaseElementAndClear and ScriptElement::execute BOTH calls m_cachedScript->removeClient(). Are you sure we want to do this? > > Yes, they are different clients. releaseElementAndClear removes the PendingScript client and execute removes the ScriptElement client. OK, now I see it. r=me.
Comment on attachment 77908 [details] Patch Rejecting attachment 77908 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-sf-cq', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 2 Last 500 characters of output: hbc/WebCorePrefix.h -c /Projects/CommitQueue/WebCore/html/HTMLVideoElement.cpp -o /Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/HTMLVideoElement.o ** BUILD FAILED ** The following build commands failed: WebCore: CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/HTMLScriptRunner.o /Projects/CommitQueue/WebCore/html/parser/HTMLScriptRunner.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2 (1 failure) Full output: http://queues.webkit.org/results/7345060
Attachment 77908 [details] did not build on win: Build output: http://queues.webkit.org/results/7266406
Created attachment 77924 [details] Patch for landing
> ** BUILD FAILED ** Was a merge error with http://trac.webkit.org/changeset/74995. Just needed to rename a new call to setElement.
The commit-queue encountered the following flaky tests while processing attachment 77924 [details]: http/tests/xmlhttprequest/basic-auth.html bug 51613 (author: ap@webkit.org) The commit-queue is continuing to process your patch.
Comment on attachment 77924 [details] Patch for landing Clearing flags on attachment: 77924 Committed r75045: <http://trac.webkit.org/changeset/75045>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/75045 might have broken Leopard Intel Debug (Tests)