Overhaul ProfileBuffer::StreamSamplesToJSON

RESOLVED FIXED in Firefox 56

Status

()

Core
Gecko Profiler
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 months ago
The profiler writes ProfileBuffer entries in a particular order, and then later
has to parse them, mostly in StreamSamplesToJSON(). That function's parsing
code is poorly structured and rather gross, at least partly because no explicit
grammar is identified.

This patch identifies the grammar in a comment, and in the same comment also
includes some examples of the more complicated subsequences. Once written down,
the grammar is obviously suboptimal -- the |Sample| entries serve no useful
purpose, for example -- but I will leave grammar improvements as follow-ups.

The patch also rewrites the parser in a more typical fashion that obviously
matches the grammar. The new parser is slightly more verbose but far easier to
understand.
(Assignee)

Comment 1

5 months ago
Created attachment 8884748 [details] [diff] [review]
Overhaul ProfileBuffer::StreamSamplesToJSON
Attachment #8884748 - Flags: review?(mstange)
(Assignee)

Comment 2

5 months ago
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5090119e51aa8f8616f1ae82a7509e488f3009ba
(Assignee)

Updated

5 months ago
Blocks: 1367406
Comment on attachment 8884748 [details] [diff] [review]
Overhaul ProfileBuffer::StreamSamplesToJSON

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

::: tools/profiler/core/ProfileBufferEntry.cpp
@@ +607,5 @@
> +//   UnsharedMemory?
> +// )*
> +//
> +// The most complicated part is the stack entry sequence that begins with
> +// CodeLocation. Here are some examples.

Thanks for the examples!

@@ +698,5 @@
> +    do { \
> +      fprintf(stderr, "ProfileBuffer parse error: %s", msg); \
> +      MOZ_ASSERT(false, msg); \
> +      goto skip_to_next_sample; \
> +    } while (0) 

end-of-line whitespace

@@ +721,5 @@
> +    }
> +  }
> +
> +  while (e.Has()) {
> +    if (e.Get().isThreadId()) {

Should we rename these isSomething functions to IsSomething? The uppercase "Get" looks a bit strange next to the lowercase "is".

@@ +849,5 @@
> +    sample.mStack = stack.GetOrAddIndex();
> +
> +    while (e.Has()) {
> +      if (e.Get().isMarker()) {
> +        e.Next();

Please add a comment here that says that we're skipping markers here because markers are streamed by a different method, StreamMarkersToJSON.

@@ +889,1 @@
>    int currentThreadID = -1;

Could we have a nested loop here as well? Mostly for consistency with the other method.
Attachment #8884748 - Flags: review?(mstange) → review+
(Assignee)

Comment 4

5 months ago
> > +    if (e.Get().isThreadId()) {
> 
> Should we rename these isSomething functions to IsSomething? The uppercase
> "Get" looks a bit strange next to the lowercase "is".

Good idea. I'll do it as a follow-up.
(Assignee)

Comment 5

5 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/903992e46e072238158b289a6bf494280e478a13
Bug 1379565 - Overhaul ProfileBuffer::StreamSamplesToJSON. r=mstange.

Comment 6

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/903992e46e07
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.