1417976 - Include docshell information in marker payloads

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement, P1)

Description

[Greg Tatum [:gregtatum] (REO Fx132)]

When attempting to profile a single page, it would be nice to filter on a specific docshell so we can exclude markers that come from the background. I'm really not familiar with the architecture of the browser enough to suggest a particular mechanism, but I want some kind of ability to associate a marker correctly.

(This is also a re-filing of bug 1109343 from the DevTools performance panel)

Comment 1

[Greg Tatum [:gregtatum] (REO Fx132)]

Also this would be nice info to have as we move away from DocShell markers to using the Gecko Profiler's markers exclusively which are associated with a specific marker.

Comment 2

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

I'm going to take a stab at this.

Comment 3

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1417976 - Part 1: Store the information of DocShells in CorePS

Added a mechanism to register and unregister the DocShells from the CorePS depending
on the state of the profiler. Registering mechanism is straightforward. During
unregistration, if profiler is not active, we remove the DocShell information
immediately. If profiler is active, we don't remove and we keep the profiler buffer
position at that moment. During another DocShell registration we Discard the
unregistered DocShells. If the profiler buffer position is greater than the position
when we captured during unregistration, we delete the DocShell since that means there
can't be any markers associated to this DocShell anymore.

Review commit: https://reviewboard.mozilla.org/r/260966/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260966/

Comment 4

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1417976 - Part 2: Include DocShell IDs to marker payloads

Review commit: https://reviewboard.mozilla.org/r/260968/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260968/

Comment 5

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1417976 - Part 3: Stream the DocShell list and DocShellId marker data to profile data

Review commit: https://reviewboard.mozilla.org/r/260970/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260970/

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8997001 [details]
Bug 1417976 - Part 1: Store the information of DocShells in CorePS

https://reviewboard.mozilla.org/r/260966/#review268362

::: docshell/base/nsDocShell.cpp:423
(Diff revision 1)
>    MOZ_ASSERT(!mObserved);
>  
>    // Avoid notifying observers while we're in the dtor.
>    mIsBeingDestroyed = true;
>  
> +  profiler_unregister_docshell(mHistoryID);

I think this call needs to be #ifdef'd.

::: tools/profiler/core/RegisteredDocShell.h:14
(Diff revision 1)
> +// while that DocShell is running and registered with the profiler, but
> +// regardless of whether the profiler is running. All accesses to it are

This comment should be tweaked a bit. A DocShell isn't "running" - it exists or it doesn't exist (gets destroyed). And we hold on to this information even after the DocShell gets destroyed.

::: tools/profiler/core/platform.cpp:316
(Diff revision 1)
> +  static void UnregisterDocshell(PSLockRef, const nsID& aRegisteredDocShellId, uint64_t aBufferPosition) {
> +    for (size_t i = 0; i < sInstance->mRegisteredDocShells.Length(); i++) {
> +      const mozilla::UniquePtr<RegisteredDocShell>& docshell = sInstance->mRegisteredDocShells[i];
> +      if (docshell->DocShellId() == aRegisteredDocShellId) {
> +        docshell->NotifyUnregistered(aBufferPosition);
> +      }
> +    }
> +  }
> +
> +  static void DiscardExpiredDocshells(PSLockRef, uint64_t aBufferPosition)

Let's spell docShell with an uppercase S everywhere, for consistency. This also applies to the other patches.

::: tools/profiler/core/platform.cpp:3406
(Diff revision 1)
> +
> +void
> +profiler_unregister_docshell(const nsID& aRegisteredDocShellId)
> +{
> +  if (!CorePS::Exists()) {
> +    // This function can be called after the main thread has already shut down.

Can it? I'd hope not. Maybe convert this into a MOZ_RELEASE_ASSERT and see how things go?

::: tools/profiler/core/platform.cpp:3416
(Diff revision 1)
> +  if (ActivePS::Exists(lock)) {
> +    CorePS::UnregisterDocshell(lock, aRegisteredDocShellId, ActivePS::Buffer(lock).mRangeEnd);
> +    return;
> +  }
> +
> +  CorePS::RemoveRegisteredDocShell(lock, aRegisteredDocShellId);

I'd slightly prefer this to be written as if / else instead of with an early return.

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8997002 [details]
Bug 1417976 - Part 2: Include DocShell IDs to marker payloads

https://reviewboard.mozilla.org/r/260968/#review268372

::: dom/base/nsDOMNavigationTiming.cpp:102
(Diff revision 1)
> -  PROFILER_TRACING("Navigation", "Unload", TRACING_INTERVAL_START);
> +  PROFILER_TRACING("Navigation", "Unload", TRACING_INTERVAL_START,
> +    mDocShell ? Some(mDocShell->HistoryID()) : Nothing());

It would be good to have a PROFILER_TRACING_DOCSHELL macro somewhere, so that the null check doesn't have to be repeated so much. Maybe nsDocShell.h would be the right place for such a macro?

::: dom/performance/Performance.cpp:250
(Diff revision 1)
>      new PerformanceMark(GetParentObject(), aName, Now());
>    InsertUserEntry(performanceMark);
>  
>  #ifdef MOZ_GECKO_PROFILER
>    if (profiler_is_active()) {
> +    Maybe<nsID> docshellId;

uppercase S please, here and elsewhere in the patch

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8997003 [details]
Bug 1417976 - Part 3: Stream the DocShell list and DocShellId marker data to profile data

https://reviewboard.mozilla.org/r/260970/#review268376

::: tools/profiler/core/ProfilerMarkerPayload.cpp:40
(Diff revision 1)
>    MOZ_ASSERT(aMarkerType);
>    aWriter.StringProperty("type", aMarkerType);
>    WriteTime(aWriter, aProcessStartTime, mStartTime, "startTime");
>    WriteTime(aWriter, aProcessStartTime, mEndTime, "endTime");
> +  if (mDocShellId) {
> +    aWriter.StringProperty("docshellId", mDocShellId->ToString());

uppercase S, here and elsewhere in the patch

::: tools/profiler/core/platform.cpp:1836
(Diff revision 1)
>    }
>    aWriter.EndObject();
>  }
>  
> +static void
> +StreamDocshell(PSLockRef aLock, SpliceableJSONWriter& aWriter)

It streams multiple DocShells, so let's call it StreamDocShells with an s at the end.

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8997001 [details]
Bug 1417976 - Part 1: Store the information of DocShells in CorePS

https://reviewboard.mozilla.org/r/260966/#review268378

::: tools/profiler/core/RegisteredDocShell.h:28
(Diff revision 1)
> +  RegisteredDocShell(const nsID& aDocShellId, const nsString& aName, const nsCString& aUrl);
> +
> +  ~RegisteredDocShell();
> +
> +  size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const;
> +  void AppendElement(SpliceableJSONWriter& aWriter);

I think StreamJSON would be a better name here.

Comment 10

[Markus Stange [:mstange]]

This looks great, nice work!

Comment 11

[Markus Stange [:mstange]]

Oh, we also need some perf.html changes for this, before we can land it: The privacy agreement needs to state that the profile now contains all URLs of all documents in all open tabs. And we "upload without URLs" should purge those URLs.

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8997001 [details]
Bug 1417976 - Part 1: Store the information of DocShells in CorePS

https://reviewboard.mozilla.org/r/260966/#review268390

I've tested the try build at https://treeherder.mozilla.org/#/jobs?repo=try&revision=59d8d0abd7cfdb2286e0169d433adc237b3b357d and the docshells list in the JSON always only contained one docshell per process, even after I had loaded lots of tabs. I think something's causing us to throw DocShells out too quickly.

I tested the patch by starting the profiler using the add-on, and then dumping the profile to a file by executing this on the Error Console: Services.profiler.dumpProfileToFileAsync("/Users/mstange/Desktop/profile-with-docshells.json")

Is that try build expected to work or was I testing an outdated build?

::: docshell/base/nsDocShell.cpp:9139
(Diff revision 1)
>  
>    return *aResult ? NS_OK : NS_ERROR_FAILURE;
>  }
>  
>  NS_IMETHODIMP
>  nsDocShell::InternalLoad(nsIURI* aURI,

I'm not sure that this is the right place to register the DocShell. I think this method can be called more than once.

::: docshell/base/nsDocShell.cpp:9194
(Diff revision 1)
>    }
>  
> +#ifdef MOZ_GECKO_PROFILER
> +  nsAutoCString spec;
> +  aURI->GetSpec(spec);
> +  profiler_register_docshell(mHistoryID, mName, spec);

What is mName? I can't find an mName field in nsDocShell.

::: tools/profiler/core/platform.cpp:300
(Diff revision 1)
>        [&](UniquePtr<RegisteredThread>& rt) { return rt.get() == aRegisteredThread; });
>    }
>  
> +  static void AppendRegisteredDocShell(PSLockRef, UniquePtr<RegisteredDocShell>&& aRegisteredDocShell)
> +  {
> +    sInstance->mRegisteredDocShells.AppendElement(std::move(aRegisteredDocShell));

We should probably have an assert here that checks (in debug builds) whether a DocShell with this ID is already in the list, to catch possible duplicates.

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 8997001 [details]
Bug 1417976 - Part 1: Store the information of DocShells in CorePS

https://reviewboard.mozilla.org/r/260966/#review268398

There's one more thing we need to take care of:

::: tools/profiler/core/platform.cpp:316
(Diff revision 1)
> +  GetRegisteredDocShells(PSLockRef)
> +  {
> +    return sInstance->mRegisteredDocShells;
> +  }
> +
> +  static void UnregisterDocshell(PSLockRef, const nsID& aRegisteredDocShellId, uint64_t aBufferPosition) {

Hmm, so the DocShell stays in an array called mRegisteredDocShells even after it is unregistered. I think that's a bit confusing.

I also realized that this patch doesn't handle the case where the profiler was running when a DocShell was unregistered, and then the profiler is stopped. Once the profiler is stopped, the profiler needs to forget about all DocShells that have been unregistered. The next time the profiler is started, there will be a new profiler buffer, and if we hold on to unregistered DocShells we will compare buffer positions from different buffers and the results are bound to be wrong.

How about this: You could add an array mUnregisteredDocShells to ActivePS (not CorePS): this array would carry all DocShells that no longer exist but which are still being referenced by profiler markers. CorePS::mRegisteredDocShells would only contain the DocShells which are still alive and registered. And when the profiler is stopped, ActivePS will be destroyed, and the list of unregistered DocShells will be cleared / destroyed automatically.

Comment 14

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

(In reply to Markus Stange [:mstange] from comment #12)
> Comment on attachment 8997001 [details]
> Bug 1417976 - Part 1: Store the information of DocShells in CorePS
> 
> https://reviewboard.mozilla.org/r/260966/#review268390
> 
> I've tested the try build at
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=59d8d0abd7cfdb2286e0169d433adc237b3b357d and the
> docshells list in the JSON always only contained one docshell per process,
> even after I had loaded lots of tabs. I think something's causing us to
> throw DocShells out too quickly.
> 
> I tested the patch by starting the profiler using the add-on, and then
> dumping the profile to a file by executing this on the Error Console:
> Services.profiler.dumpProfileToFileAsync("/Users/mstange/Desktop/profile-
> with-docshells.json")
> 
> Is that try build expected to work or was I testing an outdated build?


That should be the latest one(except some cleanups probably). I'm checking this to see what's wrong. Also addressing the review comments.

(In reply to Markus Stange [:mstange] from comment #13)
> Comment on attachment 8997001 [details]
> Bug 1417976 - Part 1: Store the information of DocShells in CorePS
> 
> https://reviewboard.mozilla.org/r/260966/#review268398
> 
> There's one more thing we need to take care of:
> 
> ::: tools/profiler/core/platform.cpp:316
> (Diff revision 1)
> > +  GetRegisteredDocShells(PSLockRef)
> > +  {
> > +    return sInstance->mRegisteredDocShells;
> > +  }
> > +
> > +  static void UnregisterDocshell(PSLockRef, const nsID& aRegisteredDocShellId, uint64_t aBufferPosition) {
> 
> Hmm, so the DocShell stays in an array called mRegisteredDocShells even
> after it is unregistered. I think that's a bit confusing.
> 
> I also realized that this patch doesn't handle the case where the profiler
> was running when a DocShell was unregistered, and then the profiler is
> stopped. Once the profiler is stopped, the profiler needs to forget about
> all DocShells that have been unregistered. The next time the profiler is
> started, there will be a new profiler buffer, and if we hold on to
> unregistered DocShells we will compare buffer positions from different
> buffers and the results are bound to be wrong.
> 
> How about this: You could add an array mUnregisteredDocShells to ActivePS
> (not CorePS): this array would carry all DocShells that no longer exist but
> which are still being referenced by profiler markers.
> CorePS::mRegisteredDocShells would only contain the DocShells which are
> still alive and registered. And when the profiler is stopped, ActivePS will
> be destroyed, and the list of unregistered DocShells will be cleared /
> destroyed automatically.

Hm, yes. You're right. I will change it to use ActivePS for unregistered DocShells.

Comment 15

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1417976 - Part 1: Store the information of DocShells in CorePS r?mstange

Added a mechanism to register and unregister the DocShells from the CorePS depending
on the state of the profiler. Registering mechanism is straightforward. During
unregistration, if profiler is not active, we remove the DocShell information
immediately. If profiler is active, we don't remove and we keep the profiler buffer
position at that moment. During another DocShell registration we Discard the
unregistered DocShells. If the profiler buffer position is greater than the position
when we captured during unregistration, we delete the DocShell since that means there
can't be any markers associated to this DocShell anymore.

MozReview-Commit-ID: IVuKQ6drvkR

Comment 16

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1417976 - Part 2: Include DocShell IDs to marker payloads r?mstange

MozReview-Commit-ID: AML1ESUnFlu

Depends on D4914

Comment 17

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1417976 - Part 3: Stream the DocShell list and DocShellId marker data to profile data r?mstange

MozReview-Commit-ID: G2s5H8i4p6E

Depends on D4915

Comment 18

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Markus, I pushed the new versions of the code to phabricator since mozreview is dead. Addressed your review comments and changed the code to use mHistoryID(which is basically a DocShell ID despite its name) and real history id(increments every page load e.g. 1, 2, 3) pair instead. It looks like we actually want that because now, we can clear all the instances of a specific DocShells with providing mHistoryID.
I changed a lot on the code and since we lost our track of review, I think a complete review would be great. Sorry for the extra work :(

This is the last try build with the patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7594788b230aff0bfa2088f28984b18086b9abc0

Comment 19

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Also I renamed the `RegisteredDocShell` class to `DocShellInformation` instead since after your proposal about keeping them on `mRegisteredDocShells` and moving them to `mDeadProfiledDocShells` after unregistration. I didn't want to use that name for dead DocShells.

Comment 20

[Markus Stange [:mstange]]

I tested the try build and it looks like the docshells for content process work correctly, but the "docShells" list in the parent process profile is always empty. Is that intentional? For example, paint markers for paints of the browser window have docshell IDs on them but those IDs aren't present in the docShells list (it's empty).

Comment 21

[Markus Stange [:mstange]]

Comment on attachment 9006188 [details]
Bug 1417976 - Part 2: Include DocShell IDs to marker payloads r?mstange

Markus Stange [:mstange] has approved the revision.

Comment 22

[Markus Stange [:mstange]]

Comment on attachment 9006189 [details]
Bug 1417976 - Part 3: Stream the DocShell list and DocShellId marker data to profile data r?mstange

Markus Stange [:mstange] has approved the revision.

Comment 23

[Markus Stange [:mstange]]

Comment on attachment 9006187 [details]
Bug 1417976 - Part 1: Store the information of DocShells in CorePS r?mstange

Markus Stange [:mstange] has approved the revision.

Comment 24

[Markus Stange [:mstange]]

@bz: This patch registers a pair (docshell, history id) with the profiler, roughly for every load, so that markers can be associated with URLs. This will allow perf.html to only show relevant markers, and to differentiate markers for root content documents from markers for iframes. Can you review the docshell changes in the patch? I've reviewed the profiler changes.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/801cdde1f597
Part 1: Store the information of DocShells in CorePS r=bzbarsky,mstange
https://hg.mozilla.org/integration/autoland/rev/44ae0b8569b2
Part 2: Include DocShell IDs to marker payloads r=mstange
https://hg.mozilla.org/integration/autoland/rev/908f30faf4b6
Part 3: Stream the DocShell list and DocShellId marker data to profile data r=mstange

Comment 26

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7fc3a3ae4b14
Backed out 3 changesets for bustages in /builds/worker/workspace/build/src/obj-firefox/dist/include/GeckoProfiler.h CLOSED TREE

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ede2fbe20d14
Part 1: Store the information of DocShells in CorePS r=bzbarsky,mstange
https://hg.mozilla.org/integration/autoland/rev/01ca16ef0b25
Part 2: Include DocShell IDs to marker payloads r=mstange
https://hg.mozilla.org/integration/autoland/rev/e70a24d50f20
Part 3: Stream the DocShell list and DocShellId marker data to profile data r=mstange

Comment 28

[Noemi Erli[:noemi_erli]]

Backed out 3 changesets (Bug 1417976) for causing devtools failures in builds/worker/workspace/build/src/tools/profiler/core/platform.cpp

Example of push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=209532950&revision=85facdfd73329fe7a95245c6deec6eecd2854451

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=209532950&repo=autoland&lineNumber=5492

Backout: https://hg.mozilla.org/integration/autoland/rev/c0536dc391c345430a421309ab67e837a89efd5d

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b00d8b617f0a
Part 1: Store the information of DocShells in CorePS r=mstange,bzbarsky
https://hg.mozilla.org/integration/autoland/rev/275d817e26df
Part 2: Include DocShell IDs to marker payloads r=mstange
https://hg.mozilla.org/integration/autoland/rev/d0421628c9b5
Part 3: Stream the DocShell list and DocShellId marker data to profile data r=mstange

Comment 30

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Fixed the problem and landed again.

Comment 31

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/b00d8b617f0a
https://hg.mozilla.org/mozilla-central/rev/275d817e26df
https://hg.mozilla.org/mozilla-central/rev/d0421628c9b5