Closed Bug 1436742 Opened 2 years ago Closed 2 years ago

Make it possible to obtain the DocGroup used to enqueue an event

Categories

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

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: jdm, Assigned: jdm)

Details

Attachments

(1 file, 1 obsolete file)

Adding this ability makes it much easier to analyze the distribution of events over documents. I have a patch I'll be attaching once I've finished rebasing it.
Attachment #8949520 - Flags: review?(nfroyd)
Assignee: nobody → josh
Status: NEW → ASSIGNED
The goal of this work is to allow interested C++ code to do the following:
>RefPtr<dom::DocGroup::Runnable> docRunnable = do_QueryObject(some_nsirunnable);
>if (docRunnable) {
>  DocGroup* group = docRunnable->DocGroup();
>  ...
>}
Comment on attachment 8949520 [details] [diff] [review]
Expose docgroup used to dispatch events when possible

Review of attachment 8949520 [details] [diff] [review]:
-----------------------------------------------------------------

Canceling review to push back on allocating another runnable wrapper; I think we should try harder to avoid more such wrappers.

::: dom/base/DocGroup.cpp
@@ +116,5 @@
>  DocGroup::Dispatch(TaskCategory aCategory,
>                     already_AddRefed<nsIRunnable>&& aRunnable)
>  {
> +  nsCOMPtr<nsIRunnable> runnable = new Runnable(Move(aRunnable), this);
> +  return mTabGroup->Dispatch(aCategory, Move(runnable.forget()));

If I am correctly tracing through all the machinery here, after this patch, going through DocGroup::Dispatch, we have:

1. Allocating a runnable wrapper so we can access the dispatching DocGroup;
2. Pass this to TabGroup::Dispatch, which is just SchedulerGroup::Dispatch.
3. SchedulerGroup::Dispatch will then allocate *another* runnable wrapper so the runnable will known its dispatching SchedulerGroup.

#3 is already kind of a pain, and adding another runnable wrapper on top of this for every labeled runnable doesn't seem like a great thing.  I know smaug has already complained about the extra allocation, and you can probably see said allocation in profiles; I imagine a second allocation would definitely start showing up.

Is there a way we could only allocate one wrapper here?  We'd have to carefully punch through some abstraction barriers, but I think that'd be OK?  Ideally we'd avoid all the above forwarding methods, too.

::: xpcom/threads/nsThread.cpp
@@ +894,5 @@
>      labeled = true;
>      MOZ_ALWAYS_TRUE(NS_SUCCEEDED(groupRunnable->GetName(aName)));
> +  } else if (RefPtr<dom::DocGroup::Runnable> docRunnable = do_QueryObject(aEvent)) {
> +    labeled = true;
> +    MOZ_ALWAYS_TRUE(NS_SUCCEEDED(docRunnable->GetName(aName)));

I realize this is not new with this patch, but one wonders why this case and the above case don't just use the nsINamed machinery (below) so we have less code to handle this...
Attachment #8949520 - Flags: review?(nfroyd)
Now with no more wrappers or allocations than previously existed.
Attachment #8949868 - Flags: review?(nfroyd)
Attachment #8949520 - Attachment is obsolete: true
Comment on attachment 8949868 [details] [diff] [review]
Expose docgroup used to dispatch events when possible

Review of attachment 8949868 [details] [diff] [review]:
-----------------------------------------------------------------

WFM, thank you!
Attachment #8949868 - Flags: review?(nfroyd) → review+
Keywords: checkin-needed
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70c39ef86701
Expose docgroup used to dispatch events when possible. r=froydnj
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/70c39ef86701
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.