Closed Bug 1417976 Opened 7 years ago Closed 6 years ago

Include docshell information in marker payloads

Categories

(Core :: Gecko Profiler, enhancement, P1)

Product:
Core
Component:
Gecko Profiler
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla65
Tracking Flags:
Tracking Status
firefox65 --- fixed

People

(Reporter: gregtatum, Assigned: canova)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 3 obsolete files)

Bug 1417976 - Part 1: Store the information of DocShells in CorePS r?mstange
46 bytes, text/x-phabricator-request
mstange
: review+
Details | Review
Bug 1417976 - Part 2: Include DocShell IDs to marker payloads r?mstange
46 bytes, text/x-phabricator-request
mstange
: review+
Details | Review
Bug 1417976 - Part 3: Stream the DocShell list and DocShellId marker data to profile data r?mstange
46 bytes, text/x-phabricator-request
mstange
: review+
Details | Review 
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)
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.
Priority: -- → P1
Blocks: 1421651
I'm going to take a stab at this.
Assignee: nobody → canaltinova
Status: NEW → ASSIGNED
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.
Attachment #8997001 - Flags: review?(mstange) → review+
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
Attachment #8997002 - Flags: review?(mstange) → review+
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.
Attachment #8997003 - Flags: review?(mstange) → review+
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.
This looks great, nice work!
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 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.
Attachment #8997001 - Flags: review+
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.
(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.
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
Attachment #8997001 - Attachment is obsolete: true
Attachment #8997003 - Attachment is obsolete: true
Attachment #8997002 - Attachment is obsolete: true
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
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.
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 on attachment 9006188 [details]
Bug 1417976 - Part 2: Include DocShell IDs to marker payloads r?mstange

Markus Stange [:mstange] has approved the revision.
Attachment #9006188 - Flags: review+
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.
Attachment #9006189 - Flags: review+
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.
Attachment #9006187 - Flags: review+
@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.
Pushed by canaltinova@gmail.com:
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
Backout by ebalazs@mozilla.com:
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
Pushed by canaltinova@gmail.com:
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
Pushed by canaltinova@gmail.com:
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
Fixed the problem and landed again.
Flags: needinfo?(canaltinova)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: