1819767 - DebuggerNotificationManager::ForDispatch shows up a tiny bit in the performance profiles

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

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

https://browserbench.org/Speedometer2.1/InteractiveRunner.html VueJS-TodoMVC
https://share.firefox.dev/3y1R9Ej

I know that isn't much in the profile, but this is some devtools code which shouldn't show up there at all when devtools aren't open. (and yes I reviewed that code :) )
And this is very hot code.

Do we have a way to quickly know if devtools are open, and if not, not run
https://searchfox.org/mozilla-central/rev/f7edb0b474a1a922f3285107620e802c6e19914d/dom/events/EventListenerManager.cpp#1302 at all?
That line could be perhaps Maybe<EventCallbackDebuggerNotificationGuard>

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

There's https://searchfox.org/mozilla-central/rev/f7edb0b474a1a922f3285107620e802c6e19914d/docshell/base/BrowsingContext.cpp#3157-3161

Comment 2

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

Even that requires some pointer chasing. Do you think it was possible to have some process level flag somewhere?
Something like MayHaveDevtoolsActive(), and I think it would be fine for it to return false only if devtools never ever have been watching anything in that process.

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #2)

Even that requires some pointer chasing. Do you think it was possible to have some process level flag somewhere?
Something like MayHaveDevtoolsActive(), and I think it would be fine for it to return false only if devtools never ever have been watching anything in that process.

That seems fine to me, maybe checking the reverse condition (e.g. DevToolsNeverOpened or something similar)
I know Julian worked on some ways for the browser to get such information, maybe he'll have ideas about it

Comment 4

[Julian Descottes [:jdescottes]]

I can't think of anything which would work as is on a per-process basis. I imagine we could have a singleton similar to our DevToolsSocketStatus, and ping it from the devtools-server, but I imagine that would not be fast enough.

Otherwise, maybe there's a way to check if the dedicated DevTools global used to load devtools modules has already been used? Or we could at least use this as an entry point to flip a flag (https://searchfox.org/mozilla-central/rev/3002762e41363de8ee9ca80196d55e79651bcb6b/js/xpconnect/loader/mozJSModuleLoader.cpp#479)

Olli: do you have an example of a similar flag, to see how we could do that for DevTools?

Comment 5

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

Could we just have a atomic flag somewhere in a process? If devtools in any way touches the process, it would flip that flag.

Comment 6

[Alexandre Poirot [:ochameau]]

olli, Do you have any suggestion on which class where to have such process-wide atomic flag?
We could probably toggle this flag from our JS codebase, so we would need to be able to expose a setter via an IDL or something.

Comment 7

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

Maybe it could be even just in ChromeUtils? That is exposed to JS too, and for C++ there could be some simple static getter.

Comment 8

[Julian Descottes [:jdescottes]]

Alex offers to try and pick this one up :)

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1819767 - [devtools] Expose ChromeUtils.isDevToolsOpened to know if DevTools are debugging something in the current process.

This doesn't help know what particular resource DevTools is currently inspecting,
but at least it helps know if it debugs something:

• one or many BrowsingContext(s) for regular DevTools (you can use BrowsingContext.watchedByDevTools instead)
• the whole process for the Browser Console/Toolbox

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1819767 - [devtools] Avoid instantiating EventCallbackDebuggerNotificationGuard when DevTools aren't opened at all.

Note that it may still be instantiated if DevTools are debugging another document/context.
Enabling this codepath only if the related document is debugged by devtools
would require checking BrowsingContext.isWatchedByDevTools.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91e577f436e9
[devtools] Expose ChromeUtils.isDevToolsOpened to know if DevTools are debugging something in the current process. r=smaug,devtools-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/f67a63d151f5
[devtools] Avoid instantiating EventCallbackDebuggerNotificationGuard when DevTools aren't opened at all. r=smaug

Comment 12

[Natalia Csoregi [:nataliaCs]]

Backed out for causing bustage on ChromeUtils.cpp

• backout: https://hg.mozilla.org/integration/autoland/rev/9592d9865f5a6c82707790d54d38667ea0ad5ea9
• push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=N1dy1FSMTTODLLYBMj6NIg.0&revision=f67a63d151f58bab4bacd9f22f0ec9ec59de58c7
• failure log: https://treeherder.mozilla.org/logviewer?job_id=410894114&repo=autoland&lineNumber=13699

[task 2023-03-30T23:12:51.035Z] 23:12:51     INFO -  /builds/worker/checkouts/gecko/dom/base/ChromeUtils.cpp: In static member function 'static void mozilla::dom::ChromeUtils::NotifyDevToolsClosed(mozilla::dom::GlobalObject&)':
[task 2023-03-30T23:12:51.035Z] 23:12:51    ERROR -  /builds/worker/checkouts/gecko/dom/base/ChromeUtils.cpp:1882:48: error: comparison of unsigned expression >= 0 is always true [-Werror=type-limits]
[task 2023-03-30T23:12:51.036Z] 23:12:51     INFO -     MOZ_ASSERT(ChromeUtils::sDevToolsOpenedCount >= 0);
[task 2023-03-30T23:12:51.036Z] 23:12:51     INFO -                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
[task 2023-03-30T23:12:51.036Z] 23:12:51     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Assertions.h:374:58: note: in definition of macro 'MOZ_VALIDATE_ASSERT_CONDITION_TYPE'
[task 2023-03-30T23:12:51.036Z] 23:12:51     INFO -           mozilla::detail::AssertionConditionType<decltype(x)>::isValid, \
[task 2023-03-30T23:12:51.037Z] 23:12:51     INFO -                                                            ^
[task 2023-03-30T23:12:51.037Z] 23:12:51     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Assertions.h:413:31: note: in expansion of macro 'MOZ_ASSERT_HELPER1'
[task 2023-03-30T23:12:51.037Z] 23:12:51     INFO -   #define MOZ_ASSERT_GLUE(a, b) a b
[task 2023-03-30T23:12:51.038Z] 23:12:51     INFO -                                 ^
[task 2023-03-30T23:12:51.039Z] 23:12:51     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Assertions.h:421:5: note: in expansion of macro 'MOZ_ASSERT_GLUE'
[task 2023-03-30T23:12:51.039Z] 23:12:51     INFO -       MOZ_ASSERT_GLUE(                                                    \
[task 2023-03-30T23:12:51.039Z] 23:12:51     INFO -       ^~~~~~~~~~~~~~~
[task 2023-03-30T23:12:51.039Z] 23:12:51     INFO -  /builds/worker/checkouts/gecko/dom/base/ChromeUtils.cpp:1882:3: note: in expansion of macro 'MOZ_ASSERT'
[task 2023-03-30T23:12:51.039Z] 23:12:51     INFO -     MOZ_ASSERT(ChromeUtils::sDevToolsOpenedCount >= 0);
[task 2023-03-30T23:12:51.040Z] 23:12:51     INFO -     ^

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b9bc293ad584
[devtools] Expose ChromeUtils.isDevToolsOpened to know if DevTools are debugging something in the current process. r=smaug,devtools-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/09d1d5558c4d
[devtools] Avoid instantiating EventCallbackDebuggerNotificationGuard when DevTools aren't opened at all. r=smaug

Comment 14

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/b9bc293ad584
https://hg.mozilla.org/mozilla-central/rev/09d1d5558c4d