Closed
Bug 1386751
Opened 7 years ago
Closed 7 years ago
Expose the PseudoStack entry to ProfilerStackCollector
Categories
(Core :: Gecko Profiler, enhancement)
Core
Gecko Profiler
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?
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(n.nethercote)
Comment 1•7 years ago
|
||
I'd have to see the patch to be certain, but it sounds reasonable.
Flags: needinfo?(n.nethercote)
Assignee | ||
Comment 2•7 years ago
|
||
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 3•7 years ago
|
||
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+
Assignee | ||
Comment 4•7 years ago
|
||
(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
Assignee | ||
Comment 5•7 years ago
|
||
MozReview-Commit-ID: 4LNuJQtm95K
Attachment #8895406 -
Flags: review?(n.nethercote)
Assignee | ||
Updated•7 years ago
|
Attachment #8893452 -
Attachment is obsolete: true
Assignee | ||
Comment 6•7 years ago
|
||
Forgot to change to MOZ_ASSERT_IF in the last patch MozReview-Commit-ID: 4LNuJQtm95K
Attachment #8895457 -
Flags: review?(n.nethercote)
Assignee | ||
Updated•7 years ago
|
Attachment #8895406 -
Attachment is obsolete: true
Attachment #8895406 -
Flags: review?(n.nethercote)
Comment 7•7 years ago
|
||
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+
Assignee | ||
Comment 8•7 years ago
|
||
(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*
Assignee | ||
Comment 9•7 years ago
|
||
MozReview-Commit-ID: 4LNuJQtm95K
Assignee | ||
Updated•7 years ago
|
Attachment #8895457 -
Attachment is obsolete: true
Comment 10•7 years ago
|
||
> 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 :)
Comment 11•7 years ago
|
||
Pushed by michael@thelayzells.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f109bc1d99be Split CollectCodeLocation into CollectWasmFrame and CollectPseudoEntry, r=njn
Comment 12•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f109bc1d99be
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•