Consider doing FlushPendingLinkUpdatesFromRunnable as an idle callback, not a runnable

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM
P2
normal
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: bz, Assigned: Away for a while)

Tracking

(Blocks: 2 bugs)

53 Branch
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(1 attachment)

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.
In fact, maybe we can even remove all our sync FlushPendingLinkUpdates calls (from the style machinery)...
Blocks: 1361618
I think I've pretty much convinced myself that we in fact can, and should, remove all sync FlushPendingLinkUpdates calls...
Priority: -- → P2
Whiteboard: [qf] → [qf:p1]
(Assignee)

Comment 3

5 months ago
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?
I assume this would use idle dispatch + timeout, once that is available for C++.
Depends on: 1358476
Blocks: 1364015
(Assignee)

Updated

5 months ago
Assignee: nobody → ehsan
(Assignee)

Comment 5

5 months ago
(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?
(Assignee)

Comment 6

5 months ago
Created attachment 8872113 [details] [diff] [review]
Move FlushPendingLinkUpdatesFromRunnable to idle dispatch
Attachment #8872113 - Flags: review?(bugs)
(Assignee)

Comment 7

5 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5fa627ebfa2e98fb8d30b6691cd9e4d0e2cd86c
Attachment #8872113 - Flags: review?(bugs) → review+
This would be a good place for NewIdleRunnableMethodWithTimer.
Ah, indeed.
(Assignee)

Comment 10

5 months ago
Oh nice, I didn't know we had that.  I will use it here.
(Assignee)

Comment 11

5 months ago
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...
Flags: needinfo?(afarre)
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.
Flags: needinfo?(afarre)
It would be nice if the API could be used also without dummy SetDeadline
I think a NewCancelableRunnableWithTimeout would be straightforward to add.
I would just reuse the name NewIdleRunnableMethodWithTimer and make the need for SetDeadline optional.
> 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...
(Assignee)

Comment 17

5 months ago
For now I will go ahead with my original patch here...
Right, what we do for the async path is kind of independent of what we do for the sync callsites, I think.
(Assignee)

Comment 19

5 months ago
(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

5 months ago
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/acca723541c2
Move FlushPendingLinkUpdatesFromRunnable to idle dispatch; r=smaug

Comment 21

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/acca723541c2
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.