Closed Bug 1386751 Opened 7 years ago Closed 7 years ago

Expose the PseudoStack entry to ProfilerStackCollector

Categories

(Core :: Gecko Profiler, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: nika, Assigned: nika)

References

Details

Attachments

(1 file, 3 obsolete files)

ProfilerStackCollector which was introduced in bug 1380286 was added so that BHR could use the profiler's interleaved stack functionality for bug 1367406. It exposes the same data which is collected from PseudoStacks for the profiler.

Unfortunately, that data is a little larger than we need. For example, the dynamic strings which are generated and then stored for the profiler include the full path to the JS file which hung, e.g. resource://.../browser.js, while the pseudostack currently only stores a partial path in order to decrease stringbuffer usage.

It would be nice to, instead of having a CollectCodeLocation callback which is called for pseudostack entries after some processing, as well as for WASM frames, have a CollectWASMFrame and CollectPseudoEntry methods which are separate. CollectPseudoEntry would have an API which could look something like:

void
CollectPseudoEntry(const js::ProfileEntry& entry);

BHR would then be able to do custom work on the ProfileEntry objects passed in and get exactly the information which it needs, and in the profiler the existing code in AddPseudoEntry would move into CollectPseudoEntry

njn, how would you feel about these changes?
Flags: needinfo?(n.nethercote)
I'd have to see the patch to be certain, but it sounds reasonable.
Flags: needinfo?(n.nethercote)
This is my first pass at what this new API would look like.

I don't support the `Privacy` feature with this patch yet. This is because with
the new API there is no way for the code to access the features flags. We don't
pass aFeatures in and we don't pass in aLock, so we can't query ActivePS
anymore.

I don't know how to fix this yet. There are plenty of different ways to handle
this, for example: we could add a field with the feature flags on ProfileBuffer
which we can query, or we could create a temporary wrapper type which holds a
pointer to the ProfileBuffer which is the actual implementer of
ProfilerStackCollector, which holds the temporary information which we need to
actually perform the collection (like with a closure).

I'd love some feedback on the basic design, as well as what you think the best
way to handle the privacy feature would be.

MozReview-Commit-ID: 4LNuJQtm95K
Attachment #8893452 - Flags: feedback?(n.nethercote)
Comment on attachment 8893452 [details] [diff] [review]
Split CollectCodeLocation into CollectWasmFrame and CollectPseudoEntry

Review of attachment 8893452 [details] [diff] [review]:
-----------------------------------------------------------------

For the handling of features, I'd do the dumb and obvious thing: just pass aFeatures to CollectPseudoEntry()! I don't see why that wouldn't work.

::: tools/profiler/core/platform.cpp
@@ +933,5 @@
>        if (pseudoEntry.kind() != js::ProfileEntry::Kind::CPP_MARKER_FOR_JS) {
> +        if (pseudoEntry.isJs() && pseudoEntry.script() && !pseudoEntry.pc()) {
> +          // The JIT only allows the top-most entry to have a nullptr pc.
> +          MOZ_ASSERT(&pseudoEntry == &racyInfo->entries[racyInfo->stackSize() - 1]);
> +        }

Please use MOZ_ASSERT_IF to ensure the condition isn't executed in opt builds. (With the current code the compiler might optimize it away, but it also might not.)
Attachment #8893452 - Flags: feedback?(n.nethercote) → feedback+
(In reply to Nicholas Nethercote [:njn] from comment #3)
> For the handling of features, I'd do the dumb and obvious thing: just pass
> aFeatures to CollectPseudoEntry()! I don't see why that wouldn't work.

Fair, it seemed like an unnecessary extra parameter for external consumers, but I suppose it's fine :)

> ::: tools/profiler/core/platform.cpp
> @@ +933,5 @@
> >        if (pseudoEntry.kind() != js::ProfileEntry::Kind::CPP_MARKER_FOR_JS) {
> > +        if (pseudoEntry.isJs() && pseudoEntry.script() && !pseudoEntry.pc()) {
> > +          // The JIT only allows the top-most entry to have a nullptr pc.
> > +          MOZ_ASSERT(&pseudoEntry == &racyInfo->entries[racyInfo->stackSize() - 1]);
> > +        }
> 
> Please use MOZ_ASSERT_IF to ensure the condition isn't executed in opt
> builds. (With the current code the compiler might optimize it away, but it
> also might not.)

Can do
MozReview-Commit-ID: 4LNuJQtm95K
Attachment #8895406 - Flags: review?(n.nethercote)
Attachment #8893452 - Attachment is obsolete: true
Forgot to change to MOZ_ASSERT_IF in the last patch

MozReview-Commit-ID: 4LNuJQtm95K
Attachment #8895457 - Flags: review?(n.nethercote)
Attachment #8895406 - Attachment is obsolete: true
Attachment #8895406 - Flags: review?(n.nethercote)
Comment on attachment 8895457 [details] [diff] [review]
Split CollectCodeLocation into CollectWasmFrame and CollectPseudoEntry

Review of attachment 8895457 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/core/ProfileBuffer.cpp
@@ +183,5 @@
> +  const char* dynamicString = aEntry.dynamicString();
> +  int lineno = -1;
> +
> +  // XXX: it's unclear why the computation of lineno should depend on
> +  // |dynamicString|. Perhaps it shouldn't?

This patch is out of date. I changed AddPseudoEntry in bug 1385197 (sorry!). Please make sure that you update this function to preserve those changes.

::: tools/profiler/core/ProfileBuffer.h
@@ +103,5 @@
> + * the collector for MergeStacks by ProfileBuffer. It holds a reference to the
> + * buffer, as well as additional feature flags which are needed to control the
> + * data collection strategy
> + */
> +class ProfileBufferCollector final : public ProfilerStackCollector

I see that you added this class to avoid passing aFeatures to CollectPseudoEntry. IMO the complexity introduced by this indirection outweighs the benefit of avoiding that argument, but I'll let you decide what to do in the final patch.
Attachment #8895457 - Flags: review?(n.nethercote) → review+
(In reply to Nicholas Nethercote [:njn] from comment #7)
> This patch is out of date. I changed AddPseudoEntry in bug 1385197 (sorry!).
> Please make sure that you update this function to preserve those changes.

Yeah, I have an old tree which I've been working off of. I'll make sure to include the changes when I rebase.

> I see that you added this class to avoid passing aFeatures to
> CollectPseudoEntry. IMO the complexity introduced by this indirection
> outweighs the benefit of avoiding that argument, but I'll let you decide
> what to do in the final patch.

My reasoning behind avoiding the argument was just that it seemed like unnecessarily exposing an internal implementation detail of the profiler. I can switch to using the extra argument, but I didn't like how it affected the public API of the profiler.

*shrug*
Attachment #8895457 - Attachment is obsolete: true
> My reasoning behind avoiding the argument was just that it seemed like
> unnecessarily exposing an internal implementation detail of the profiler.

Sure. If it was a widely-used function then it might be worth it, but it only has 2(?) uses and so for me a tiny bit of interface complexity is better than significantly more implementation complexity. It's a question of taste and people can reasonably disagree :)
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f109bc1d99be
Split CollectCodeLocation into CollectWasmFrame and CollectPseudoEntry, r=njn
https://hg.mozilla.org/mozilla-central/rev/f109bc1d99be
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: