1361709 - Consider doing FlushPendingLinkUpdatesFromRunnable as an idle callback, not a runnable

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Boris Zbarsky [:bzbarsky]]

Right now, if you insert a bunch of links into the DOM we queue a runnable to do the expensive work of parsing their href URIs and registering them as history observers so they can pick up their :visited state.

We could probably do this as an idle callback instead, I would think, since the history parts are kinda async anyway.

Comment 1

[Boris Zbarsky [:bzbarsky]]

In fact, maybe we can even remove all our sync FlushPendingLinkUpdates calls (from the style machinery)...

Comment 2

[Boris Zbarsky [:bzbarsky]]

I think I've pretty much convinced myself that we in fact can, and should, remove all sync FlushPendingLinkUpdates calls...

Comment 3

[(no longer active)]

Can we actually move this over to idle dispatch?  What if the user doesn't become idle for a long time?  Doesn't this mean that the visited state of the links will not get updated for a noticeable amount of time?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

I assume this would use idle dispatch + timeout, once that is available for C++.

Comment 5

[(no longer active)]

(In reply to Boris Zbarsky [:bz] (work week until 5/26) (if a patch has no decent message, automatic r-) from comment #2)
> I think I've pretty much convinced myself that we in fact can, and should,
> remove all sync FlushPendingLinkUpdates calls...

Boris, is there a bug on file for this part?

Comment 6

[(no longer active)]

Attachment: Move FlushPendingLinkUpdatesFromRunnable to idle dispatch

Comment 7

[(no longer active)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5fa627ebfa2e98fb8d30b6691cd9e4d0e2cd86c

Comment 8

[Andreas Farre [:farre]]

This would be a good place for NewIdleRunnableMethodWithTimer.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Ah, indeed.

Comment 10

[(no longer active)]

Oh nice, I didn't know we had that.  I will use it here.

Comment 11

[(no longer active)]

Hmm...  I'm not really sure how one is supposed to use this API.  I made this change:

$ git diff
diff --git a/dom/base/nsDocument.cpp b/dom/base/nsDocument.cpp
index 2b373a075dea..2551b4ee03a0 100644
--- a/dom/base/nsDocument.cpp
+++ b/dom/base/nsDocument.cpp
@@ -10009,7 +10009,8 @@ nsIDocument::RegisterPendingLinkUpdate(Link* aLink)
 
   if (!mHasLinksToUpdateRunnable) {
     nsCOMPtr<nsIRunnable> event =
-      NewRunnableMethod(this, &nsIDocument::FlushPendingLinkUpdatesFromRunnable);
+      NewIdleRunnableMethodWithTimer("nsIDocument::FlushPendingLinkUpdatesFromRunnable",
+                                     this, &nsIDocument::FlushPendingLinkUpdatesFromRunnable);
     // Do this work in a second in the worst case.
     nsresult rv =
       NS_IdleDispatchToCurrentThread(event.forget(), 1000);


And I am getting this compiler error:

$ ./mach build binaries
 0:00.12 /usr/bin/make -C /home/ehsan/moz/src.1347035/obj-ff-clang-plugin.noindex -j8 -s backend
 0:00.18 /usr/bin/make -j8 -s binaries
 0:00.46 Elapsed: 0.17s; From dist/include: Kept 5363 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.84 force-cargo-library-build
 0:01.45     Finished dev [optimized + debuginfo] target(s) in 0.2 secs
 0:11.49 In file included from /home/ehsan/moz/src.1347035/obj-ff-clang-plugin.noindex/dom/base/Unified_cpp_dom_base5.cpp:2:
 0:11.49 In file included from /home/ehsan/moz/src.1347035/dom/base/nsDOMClassInfo.cpp:25:
 0:11.49 In file included from /home/ehsan/moz/src.1347035/obj-ff-clang-plugin.noindex/dist/include/mozilla/DOMEventTargetHelper.h:19:
 0:11.49 In file included from /home/ehsan/moz/src.1347035/obj-ff-clang-plugin.noindex/dist/include/mozilla/EventListenerManager.h:11:
 0:11.49 In file included from /home/ehsan/moz/src.1347035/obj-ff-clang-plugin.noindex/dist/include/mozilla/dom/EventListenerBinding.h:10:
 0:11.49 In file included from /home/ehsan/moz/src.1347035/obj-ff-clang-plugin.noindex/dist/include/mozilla/dom/CallbackInterface.h:19:
 0:11.49 In file included from /home/ehsan/moz/src.1347035/obj-ff-clang-plugin.noindex/dist/include/mozilla/dom/CallbackObject.h:31:
 0:11.49 In file included from /home/ehsan/moz/src.1347035/dom/base/nsJSEnvironment.h:18:
 0:11.49 /home/ehsan/moz/src.1347035/obj-ff-clang-plugin.noindex/dist/include/nsThreadUtils.h:684:68: error: no member named 'SetDeadline' in 'nsIDocument'
 0:11.49   void SetDeadline(mozilla::TimeStamp aDeadline) { if (mObj) mObj->SetDeadline(aDeadline); }
 0:11.49                                                              ~~~~  ^
 0:11.49 /home/ehsan/moz/src.1347035/obj-ff-clang-plugin.noindex/dist/include/nsThreadUtils.h:1154:15: note: in instantiation of member function 'nsRunnableMethodReceiver<nsIDocument, true, true>::SetDeadline' requested here
 0:11.49     mReceiver.SetDeadline(aDeadline);
 0:11.49               ^
 0:11.49 /home/ehsan/moz/src.1347035/obj-ff-clang-plugin.noindex/dist/include/nsThreadUtils.h:1120:12: note: in instantiation of member function 'mozilla::detail::RunnableMethodImpl<nsIDocument *, void (nsIDocument::*)(), true, mozilla::RunnableKind::IdleWithTimer>::SetDeadline' requested here
 0:11.49   explicit RunnableMethodImpl(ForwardedPtrType&& aObj, Method aMethod,
 0:11.49            ^
 0:11.49 /home/ehsan/moz/src.1347035/obj-ff-clang-plugin.noindex/dist/include/nsThreadUtils.h:1376:24: note: in instantiation of function template specialization 'mozilla::detail::RunnableMethodImpl<nsIDocument *, void (nsIDocument::*)(), true, mozilla::RunnableKind::IdleWithTimer>::RunnableMethodImpl<nsIDocument *>' requested here
 0:11.49   return do_AddRef(new detail::IdleRunnableMethodWithTimerImpl<PtrType, Method>(
 0:11.49                        ^
 0:11.49 /home/ehsan/moz/src.1347035/obj-ff-clang-plugin.noindex/dist/include/nsThreadUtils.h:1387:5: note: in instantiation of function template specialization 'mozilla::NewIdleRunnableMethodWithTimer<nsIDocument *, void (nsIDocument::*)()>' requested here
 0:11.49     NewIdleRunnableMethodWithTimer(Forward<PtrType>(aPtr), aMethod),
 0:11.49     ^
 0:11.49 /home/ehsan/moz/src.1347035/dom/base/nsDocument.cpp:10012:7: note: in instantiation of function template specialization 'mozilla::NewIdleRunnableMethodWithTimer<nsIDocument *, void (nsIDocument::*)()>' requested here
 0:11.49       NewIdleRunnableMethodWithTimer("nsIDocument::FlushPendingLinkUpdatesFromRunnable",
 0:11.49       ^
 0:11.49 1 error generated.
 0:11.49 /home/ehsan/moz/src.1347035/config/rules.mk:1063: recipe for target 'Unified_cpp_dom_base5.o' failed
 0:11.49 make[2]: *** [Unified_cpp_dom_base5.o] Error 1
 0:11.49 /home/ehsan/moz/src.1347035/config/recurse.mk:73: recipe for target 'dom/base/target' failed
 0:11.49 make[1]: *** [dom/base/target] Error 2
 0:11.49 /home/ehsan/moz/src.1347035/config/recurse.mk:38: recipe for target 'binaries' failed
 0:11.49 make: *** [binaries] Error 2
 0:11.53 ccache (direct) hit rate: 0.0%; (preprocessed) hit rate: 0.0%; miss rate: 0.0%

It looks like now this setup expects me to implement an nsIDocument::SetDeadline()?  I'm not sure if that a) makes sense, because nsIDocument is such a generic class, and b) what that function is supposed to do, and c) how this setup is going to work if we have more than one of these runnables in flight per an nsIDocument (i.e., how is SetDeadline() even going to know which runnable it's being called for?)

It seems like either I'm not understanding how this API works, or this isn't the right API to use here.  The only information I could find about this is bug 1358476 comment 3 which is very vague...

Comment 12

[Andreas Farre [:farre]]

Right, you need to add a SetDeadline. The guarantee you have is that it will be called immediately before Run is called. There is also no harm in having an empty implementation of SetDeadline. 

This sorely needs more documentation than present in code. I'll make that happen.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

It would be nice if the API could be used also without dummy SetDeadline

Comment 14

[Andreas Farre [:farre]]

I think a NewCancelableRunnableWithTimeout would be straightforward to add.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

I would just reuse the name NewIdleRunnableMethodWithTimer and make the need for SetDeadline optional.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Boris, is there a bug on file for this part?

Not yet.  I removed most of the calls (all the ones that definitely couldn't affect observable behavior) in bug 1361274.  There are two calls remaining: from nsCSSFrameConstructor::ResolveStyleContext and from ElementRestyler::Restyle.  I _think_ they can both be removed but it really needs double-checking; that's why I said "pretty much" instead of "definitely" and why I didn't present an argument for why they can be removed...

Comment 17

[(no longer active)]

For now I will go ahead with my original patch here...

Comment 18

[Boris Zbarsky [:bzbarsky]]

Right, what we do for the async path is kind of independent of what we do for the sync callsites, I think.

Comment 19

[(no longer active)]

(In reply to Boris Zbarsky [:bz] (work week until 5/26) (if a patch has no decent message, automatic r-) from comment #18)
> Right, what we do for the async path is kind of independent of what we do
> for the sync callsites, I think.

Yes.  I was referring to the other conversation in the bug.  :-)  (Comment 8 onwards...)

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/acca723541c2
Move FlushPendingLinkUpdatesFromRunnable to idle dispatch; r=smaug

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/acca723541c2