Closed Bug 1819767 Opened 3 months ago Closed 2 months ago

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

Categories

(DevTools :: Debugger, defect, P2)

Product:
DevTools
Component:
Debugger
Type:
defect

Tracking

(firefox113 fixed)

Status:
RESOLVED FIXED
Milestone:
113 Branch
Tracking Flags:
Tracking Status
firefox113 --- fixed

People

(Reporter: smaug, Assigned: ochameau, NeedInfo)

References

(Blocks 1 open bug)

Details

(Whiteboard: [sp3])

Attachments

(2 files)

Bug 1819767 - [devtools] Expose ChromeUtils.isDevToolsOpened to know if DevTools are debugging something in the current process.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1819767 - [devtools] Avoid instantiating EventCallbackDebuggerNotificationGuard when DevTools aren't opened at all.
48 bytes, text/x-phabricator-request
Details | Review

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>

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.

Flags: needinfo?(nchevobbe)

(In reply to Olli Pettay [:smaug][bugs@pettay.fi] 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

Flags: needinfo?(nchevobbe) → needinfo?(jdescottes)
Whiteboard: [sp3]

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?

Flags: needinfo?(jdescottes) → needinfo?(smaug)

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

Flags: needinfo?(smaug)
Type: enhancement → defect
Priority: -- → P2

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.

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

Alex offers to try and pick this one up :)

Severity: -- → S3
Flags: needinfo?(poirot.alex)

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
Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED

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.

Pushed by apoirot@mozilla.com:
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

Backed out for causing bustage on ChromeUtils.cpp

[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 -     ^
Flags: needinfo?(poirot.alex)
Flags: needinfo?(poirot.alex)
Pushed by apoirot@mozilla.com:
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

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

Status: ASSIGNED → RESOLVED
Closed: 2 months ago
status-firefox113: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 113 Branch
You need to log in before you can comment on or make changes to this bug.