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

Status: RESOLVED FIXED

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

Description

[Josh Matthews [:jdm]]

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.

Comment 1

[Josh Matthews [:jdm]]

Attachment: Expose docgroup used to dispatch events when possible

Comment 2

[Josh Matthews [:jdm]]

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 3

[Josh Matthews [:jdm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fcb2be3f07e282b789ea3a4a7544461a86531e0 looks good.

Comment 4

[Nathan Froyd [:froydnj]]

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...

Comment 5

[Josh Matthews [:jdm]]

Attachment: Expose docgroup used to dispatch events when possible

Now with no more wrappers or allocations than previously existed.

Comment 6

[Nathan Froyd [:froydnj]]

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!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70c39ef86701
Expose docgroup used to dispatch events when possible. r=froydnj

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/70c39ef86701