1676361 - Clean up GeckoProfiler includes

Status: RESOLVED FIXED

(Core :: Gecko Profiler, task, P3)

Comment 1

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1676361 - Clean up profiler includes. r=gerald

Depends on D96538

Comment 2

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1676361 - Remove unused include for GeckoProfiler.h from generated bindings headers. r=gerald

Depends on D96545

Comment 3

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1676361 - Move UniqueProfilerBacktrace.h to separate header file. r=gerald

Depends on D96558

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Oooh, bad timing I'm afraid! I'm currently doing some big work around markers, and after bug 1675409 UniqueProfilerBacktrace won't be used anymore, ProfilerMarkerPayload.cpp will be gone, etc. So a lot of your patches won't apply anymore. And I'd rather not rebase bug 1675409 on top of them.

I appreciate that you're cleaning up these things, to reduce include file hell, that's great!
But please wait after bug 1675409 lands -- I'm planning shortly after the freeze next week. Happy to help after that.
But please ping me if it's urgent.

Comment 5

[Simon Giesecke [:sg] [he/him]]

Ah, that's true, a bit of bad timing. Retrospectively, it would have been better to ask about doing this first, but I was in a rather fine-grained process of identifying expensive headers and reducing their cost. (That being said, GeckoProfiler.h and its dependencies are included at a lot of places where they do not appear to be needed, including things that could be avoided, and costly to parse due to their heavy use of templates [1].)

I'll be happy to revisit my work on top of the patches from Bug 1675409. I'd like not to wait too long in order not to require rebasing on other changes too much, but next week is totally fine.

[1] While clearly a separate issue, I take the opportunity to share this information here: Besides header parsing time, ClangBuildAnalyzer identifies these as among the templates that are most costly to instantiate:

  4017 ms: mozilla::baseprofiler::AddMarker<mozilla::baseprofiler::markers::Text, std::__cxx11::basic_string<char>> (527 times, avg 7 ms)
  4010 ms: mozilla::baseprofiler::AddMarkerToBuffer<mozilla::baseprofiler::markers::NoPayload> (527 times, avg 7 ms)
  3932 ms: mozilla::baseprofiler::AddMarkerToBuffer<mozilla::baseprofiler::markers::Text, std::__cxx11::basic_string<char>> (527 times, avg 7 ms)
  3906 ms: mozilla::base_profiler_markers_detail::AddMarkerToBuffer<mozilla::baseprofiler::markers::NoPayload> (527 times, avg 7 ms)
  3832 ms: mozilla::base_profiler_markers_detail::AddMarkerToBuffer<mozilla::baseprofiler::markers::Text, std::__cxx11::basic_string<char>> (527 times, avg 7 ms)
  3669 ms: mozilla::base_profiler_markers_detail::AddMarkerWithOptionalStackToBuffer<mozilla::baseprofiler::markers::NoPayload> (527 times, avg 6 ms)
  3570 ms: mozilla::base_profiler_markers_detail::AddMarkerWithOptionalStackToBuffer<mozilla::baseprofiler::markers::Text, std::__cxx11::basic_string<char>> (527 times, avg 6 ms)
  3390 ms: mozilla::base_profiler_markers_detail::MarkerTypeSerialization<mozilla::baseprofiler::markers::Text>::Serialize<std::__cxx11::basic_string<char>> (527 times, avg 6 ms)

Maybe these could be explicitly instantiated instead? Or maybe if this is heavily changed by Bug 1675409 as well, I can share the new data after landing that.

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Thank you for your patience.
I'd certainly agree with reducing includes where possible. The marker works helps to separate marker-related functions from other profiler functions, so eventually this should help reduce includes as well...

About the expensive AddMarker templated functions, I'm afraid they will be used even more after bug 1675409, because I'm migrating old marker APIs to these new ones!
Maybe it would be worth re-running your analysis after that bug lands? Are there instructions somewhere so I could try it in advance?
Happy to help reduce their cost after that...

Comment 7

[Simon Giesecke [:sg] [he/him]]

(In reply to Gerald Squelart [:gerald] (he/him) from comment #6)

Thank you for your patience.
I'd certainly agree with reducing includes where possible. The marker works helps to separate marker-related functions from other profiler functions, so eventually this should help reduce includes as well...

Sounds good!

About the expensive AddMarker templated functions, I'm afraid they will be used even more after bug 1675409, because I'm migrating old marker APIs to these new ones!
Maybe it would be worth re-running your analysis after that bug lands?

I will definitely do that :)

Are there instructions somewhere so I could try it in advance?

You can use the script from https://bugzilla.mozilla.org/show_bug.cgi?id=1676346#c6 (with some local adaptations). If you have trouble running that, please contact me on Slack/Matrix :)

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

I know you're eager to land this.
You may have noticed that bug 1675409 made it to central 2f08ec7e57c3, I see you've updated your patches, but beware that it was backed out in autoland, so it will disappear from the next central!
(It's a trivial fix, so I'll re-land ASAP...)

Comment 9

[Simon Giesecke [:sg] [he/him]]

Thanks for the heads-up! I noticed the second backup already. I still need to fix Bug 1674080 before I can land this anyway. I already updated the patch to build after Bug 1675409, and removed the one relating to UniqueProfilerBacktrace, but I'll check again if there are new GeckoProfiler.h includes that could be avoided now :)

Comment 10

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1676361 - Move ExecutionContext to a separate header file to avoid pulling in GeckoProfiler.h everywhere. r=tcampbell

Depends on D97617

Comment 11

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1676361 - Move AutoEntryScript to a separate header file to avoid pulling in GeckoProfiler.h everywhere. r=mccr8

Depends on D97741

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb69d4d37b33
Clean up profiler includes. r=gerald
https://hg.mozilla.org/integration/autoland/rev/7fab6d76a6eb
Remove unused include for GeckoProfiler.h from generated bindings headers. r=gerald
https://hg.mozilla.org/integration/autoland/rev/08d5416655f3
Move ExecutionContext to a separate header file to avoid pulling in GeckoProfiler.h everywhere. r=tcampbell

Comment 13

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/bb69d4d37b33
https://hg.mozilla.org/mozilla-central/rev/7fab6d76a6eb
https://hg.mozilla.org/mozilla-central/rev/08d5416655f3

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9062e17fe15c
Move AutoEntryScript to a separate header file to avoid pulling in GeckoProfiler.h everywhere. r=mccr8

Comment 15

[Sandor Molnar[:smolnar]]

Backed out changeset https://hg.mozilla.org/integration/autoland/rev/9062e17fe15c62ba1274b5831ba8d1ef0e03edd4 , on Simon's request.

Backout link: https://hg.mozilla.org/integration/autoland/rev/41654d4eb1ac78477496960a52b57d4a550f0a63

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06452c4c828c
Move AutoEntryScript to a separate header file to avoid pulling in GeckoProfiler.h everywhere. r=mccr8

Comment 17

[Noemi Erli[:noemi_erli]]

Backed out changeset 06452c4c828c (Bug 1676361) for causing bustages CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&selectedTaskRun=AopfI47yQsubAmhnb3k2jQ.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=06452c4c828cbae9408c1d13c8c0971796f121b8

Failure log: https://treeherder.mozilla.org/logviewer?job_id=332066514&repo=autoland&lineNumber=4852

Backout: https://hg.mozilla.org/integration/autoland/rev/8e8d076438b488b70ec57167a62444f5e47778d5

Comment 18

[Simon Giesecke [:sg] [he/him]]

Fixed the non-unified-build bustages and relanded.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c6b72f3c76ba
Move AutoEntryScript to a separate header file to avoid pulling in GeckoProfiler.h everywhere. r=mccr8

Comment 20

[Alexandru Michis [:malexandru]]

Backed out changeset c6b72f3c76ba (Bug 1676361) for causing bustages in nsSocketTransportService2.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/f4464f7075d664f704635a39899dd150d06e5eff

Push with failures, failure log.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7541959e1751
Move AutoEntryScript to a separate header file to avoid pulling in GeckoProfiler.h everywhere. r=mccr8

Comment 22

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/7541959e1751