Open Bug 1369955 Opened 7 years ago Updated 2 years ago

Off-the shelf control, enumeration, and documentation of performance sources

Categories

(Core :: Gecko Profiler, enhancement, P2)

Product:
Core
Component:
Gecko Profiler
Type:
enhancement

Tracking

()

Status:
ASSIGNED

People

(Reporter: jimb, Assigned: jimb)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 4 obsolete files)

Bug 1369955: Add perfstream module. Define perfstream::Measure and Group types, with StaticGroup helper.
59 bytes, text/x-review-board-request
Details
Bug 1369955: Introduce a perfstream::Measure subtree for SpiderMonkey.
59 bytes, text/x-review-board-request
Details
Bug 1369955: Implement a JavaScript `Measure` API to the perfstream::Measure tree.
59 bytes, text/x-review-board-request
sfink
: review+
Details
Bug 1369955: Add perfstream::Measure subtree for SpiderMonkey's TraceLogger.
59 bytes, text/x-review-board-request
sfink
: review+
Details 
From bug 1355649:

Gecko should provide a common registry for enabling data sources, so that Dev need not write yet another environment variable parser, JavaScript function in js/src/builtin/TestingFunctions.cpp, WebIDL API, and Gecko Profiler add-on patch to turn the data source on and off.

The registry should be tree-structured, to allow different teams to add any number of measurements for their own component without distracting other teams by cluttering a common list with mysterious details.

Adding a simple, on-off data source to the registry should require little effort, but the design should permit incremental addition of parameters, tooltip-style summaries, documentation URLs, natural language labels, and so on.
See comments in Measure.h.
Assignee: nobody → jimb
Status: NEW → ASSIGNED
Attachment #8874079 - Flags: review?(n.nethercote)

  

  



        Attachment #8874080 -
        Flags: review?(n.nethercote)

    
    

    
  


  

  

  



        Attachment #8874081 -
        Flags: review?(n.nethercote)

    
    

    
  


  

  

  



        Attachment #8874082 -
        Flags: review?(hv1989)

    
  

        Attachment #8874079 -
        Flags: review?(n.nethercote) → feedback?(n.nethercote)

    
  

        Attachment #8874080 -
        Flags: review?(n.nethercote) → feedback?(n.nethercote)

    
  

        Attachment #8874081 -
        Flags: review?(n.nethercote) → feedback?(n.nethercote)

    
  

        Attachment #8874082 -
        Flags: review?(hv1989) → feedback?(hv1989)

    
    

    
  
njn, I believe you were a SpiderMonkey peer at one point; are you still able to review patches there? Are you comfortable doing so?

  

  



    
    

    
  
I still am a peer, thought it's a rarely-invoked privilege nowadays. I'm happy to take a look on Monday and I'll let you know my comfort level with these.

  

  



    
  

        Attachment #8874082 -
        Flags: feedback?(hv1989) → feedback?(sphink)

    
    

    
  
Steve, I've flagged you for feedback on the TraceLogger integration. If there's someone better suited to look at that, please feel free to pass the buck; I accept all blame.

  

  



    
    

    
  
Comment on attachment 8874079 [details] [diff] [review]
Add perfstream module. Define mozilla::Measures type.

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

Are these intended to be serialized/deserialized at all? eg if you want to communicate Measures to the profiler.

::: perfstream/Measure.h
@@ +204,5 @@
> +     *
> +     * A live Measure must always be safe to use, even if the inner measure
> +     * disappears. Such a Measure always reports Dropped as its state.
> +     */
> +    Dropped,

If some children are Enabled and others are Dropped, what is the parent's State? Enabled? Mixed?

Same question with s/Enabled/Disabled/g. I'm guessing that Dropped measures aren't relevant to Enabled/Disabled vs Mixed, but I don't know.

If a measure is Disabled, does that imply that it is "available" in some sense? Is it assumed that if things are #ifdef'd out, they won't show up at all? Oh. You answer this below. Since I had the question, maybe a brief mention in the comment for Disabled?

@@ +272,5 @@
> +
> +  /**
> +   * Return true if this measure is currently collecting data.
> +   */
> +  bool GetEnabled() const {

IsEnabled() sounds better to me, especially since sounds imperative to me: "go get enabled, mr. measure!" But perhaps this is for conforming to coding standards? (It's infallible, so could it be Enabled()?)

@@ +295,5 @@
> +    /**
> +     * This measure is controlled by a compile-time option; you'll need to
> +     * recompile the application to enable or disable it.
> +     */
> +    Static,

"Static" is nicely opposite Dynamic and matches the semantics, but... it's a little confusing given the overload with C++'s various meanings of "static". Would CompileTime be overly specific? (I do not feel at all strongly about this; Static is not bad.)

  

  



    
    

    
  
Comment on attachment 8874080 [details] [diff] [review]
Introduce a perfstream::Measure subtree for SpiderMonkey.

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

::: js/public/Measures.h
@@ +31,5 @@
> + * tree of Measures available for the given context's runtime. Include optional
> + * features according to options, which is the logical or of zero or more values
> + * from JS::MeasureOptions.
> + *
> + * On OOM, return a null UniquePtr.

I suspect making this infallible would be defensible, but this is ok too.

  

  



    
    

    
  
Comment on attachment 8874079 [details] [diff] [review]
Add perfstream module. Define mozilla::Measures type.

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

General thoughts:

- I like that there are lots of comments.

- At this point I've only read the first patch and it's all a bit abstract. An example tree in comments might help?

- You've used some SpiderMonkey style-isms throughout. First, Gecko classes/enums/inline functions all have their braces on the following line. Second, Gecko if/else blocks are always braced.

- Can I convince you to use C++ comments instead of C comments? :)

- I wonder if all this new code should go under tools/.

::: perfstream/GroupingOnlyMeasure.h
@@ +17,5 @@
> +/**
> + * A base class for GroupingOnly Measures, which exist only to provide tree
> + * structure and cannot be enabled or disabled directly. Subclasses only need to
> + * implement the description and child-related methods.
> + */

I don't have my head fully around this stuff yet, but GroupingOnly feels inelegant. I'm wondering if it should be possible to only define leaf nodes, and then non-leaf nodes would be automatically "filled in". This is how memory reporting works -- you measure "a/b/c" and "a/b/d" and about:memory fills in the internal 'a' and 'b' nodes.

::: perfstream/Measure.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* A tree of performance measures. */

I would remove this comment, and move the big comment that starts on line 24 here. Put the important stuff first! :)

@@ +232,5 @@
> +
> +  /**
> +   * Return true if state represents an enabled state.
> +   */
> +  static bool StateIsEnabled(State state) {

Call is IsEnabled()?

@@ +267,5 @@
> +  /**
> +   * If aEnabled is true, try to enable this Measure; otherwise, try to disable
> +   * it. Return this Measure's resulting state.
> +   */
> +  virtual State SetEnabled(bool aEnabled) = 0;

I find functions like this unclear, and would prefer to have two param-less functions: SetEnabled() and SetDisabled().

@@ +272,5 @@
> +
> +  /**
> +   * Return true if this measure is currently collecting data.
> +   */
> +  bool GetEnabled() const {

Also call this IsEnabled()? Not sure if the overloading would be confusing.

@@ +287,5 @@
> +   * mysteriously invisible.
> +   */
> +  enum class EnableKind {
> +    /**
> +     * This measure can be enabled and disable while the application runs.

s/disable/disabled/

@@ +342,5 @@
> +
> +  /**
> +   * Return the descendant of this measure named by aPath. aPath is a
> +   * period-separated path of Description::mIdentifier strings, taken relative
> +   * to this Measure. If there is no such child, return a null UniquePtr.

Hmm. Memory reporting uses '/' as a separator and I regret it, because we end up with all sorts of things in path segments, including URLs, and have to do some crummy hacks to handle them. If I were to do it again from scratch I would make each path an array or vector of strings.

@@ +428,5 @@
> +   */
> +  const char* mName;
> +
> +  /**
> +   * A translation string entity name for mName.

Is this an l10n thing? (I'm not familiar with the phrase "translation string entity".) Is l10n necessary for this feature? Will it be user-facing?

  

  



        Attachment #8874079 -
        Flags: feedback?(n.nethercote) → feedback+

    
    

    
  
Comment on attachment 8874080 [details] [diff] [review]
Introduce a perfstream::Measure subtree for SpiderMonkey.

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

::: js/src/vm/Measures.cpp
@@ +130,5 @@
> +    "SpiderMonkey",
> +    "SpiderMonkey",
> +    nullptr,
> +    "the SpiderMonkey JavaScript engine",
> +    nullptr,

Does C++ have a portable syntax for specifying the field names when initializing like this? If so, please use it. If not, please put the field names in comments, because it's really hard to know what this means without them. This applies here, and for TestMeasure above, and for other patches in this bug.

  

  



        Attachment #8874080 -
        Flags: feedback?(n.nethercote) → feedback+

    
    

    
  
Comment on attachment 8874081 [details] [diff] [review]
Implement a JavaScript `Measure` API to the perfstream::Measure tree.

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

You should get someone who's better than me with JSAPI stuff to review this :)

  

  



        Attachment #8874081 -
        Flags: feedback?(n.nethercote) → feedback+

    
    

    
  
I've done a high-level pass over all the patches. It all seems plausible and you've clearly put a lot of thought into the design. But it's still very abstract and I'm having trouble understanding how one uses all this machinery in practice. For example, what can you do with the TraceLoggerMeasure?

  

  



    
    

    
  
(In reply to Steve Fink [:sfink] [:s:] from comment #8)
> Are these intended to be serialized/deserialized at all? eg if you want to
> communicate Measures to the profiler.

Measure is only concerned with control. It doesn't have any say in how the data gets serialized, or even really what the data is. The idea is that, no matter what you're building, you can hook it up through this API.

> ::: perfstream/Measure.h
> @@ +204,5 @@
> > +     *
> > +     * A live Measure must always be safe to use, even if the inner measure
> > +     * disappears. Such a Measure always reports Dropped as its state.
> > +     */
> > +    Dropped,
> 
> If some children are Enabled and others are Dropped, what is the parent's
> State? Enabled? Mixed?
> 
> Same question with s/Enabled/Disabled/g. I'm guessing that Dropped measures
> aren't relevant to Enabled/Disabled vs Mixed, but I don't know.

Thanks; things to make clear in the comments:

- A Dropped measure refers to a source that isn't there any more. Mixed is intended for GUIs trying to reflect the state of a subtree. I think you'd want to display to reflect the nodes that are present.

- Dropped measures should not appear as children any longer; they're just gone.

> If a measure is Disabled, does that imply that it is "available" in some
> sense? Is it assumed that if things are #ifdef'd out, they won't show up at
> all? Oh. You answer this below. Since I had the question, maybe a brief
> mention in the comment for Disabled?

Yep, will clarify.

> @@ +272,5 @@
> > +
> > +  /**
> > +   * Return true if this measure is currently collecting data.
> > +   */
> > +  bool GetEnabled() const {
> 
> IsEnabled() sounds better to me, especially since sounds imperative to me:
> "go get enabled, mr. measure!" But perhaps this is for conforming to coding
> standards? (It's infallible, so could it be Enabled()?)

NJN had some suggestions about these names, too. I was trying to match what XPIDL forces you to name methods for a read/write `enable` attribute, on the grounds that that'd be most familiar to Gecko hackers.

> @@ +295,5 @@
> > +    /**
> > +     * This measure is controlled by a compile-time option; you'll need to
> > +     * recompile the application to enable or disable it.
> > +     */
> > +    Static,
> 
> "Static" is nicely opposite Dynamic and matches the semantics, but... it's a
> little confusing given the overload with C++'s various meanings of "static".
> Would CompileTime be overly specific? (I do not feel at all strongly about
> this; Static is not bad.)

I agree that Static is overused. CompileTime isn't bad.

  

  



    
    

    
  
Sorry I still haven't marked this feedback+. I've read through it, and it seems basically sane, so perhaps I should just go ahead and do it. But I haven't really made sure it makes sense in the specific case of tracelogger, and need to. But in the meantime, some questions:

- I would like to have some measures that are known to be cheap enough that they can be left enabled even in performance test runs. Is this the level at which that sort of distinction should be made?

- This feels like a lot of boilerplate for simple cases. Perhaps invoking the dark magic of macros and/or templates could trim this down?

  

  



    
    

    
  
(In reply to Steve Fink [:sfink] [:s:] from comment #15)
> - I would like to have some measures that are known to be cheap enough that
> they can be left enabled even in performance test runs. Is this the level at
> which that sort of distinction should be made?

I can think of two situations you might be referring to.

If you mean that there should be measures that are simply on by default, in all configurations, since they are valuable and cheap, then that should be fine: the Measure tree doesn't say what anything's initial state should be. what it should do is accurately reflect the current state, and let you control it.

If you mean that there are certain measures that you would like to enable during performance test runs, then I think that is what "Categories" are for. TraceLogger has the categories "Default", "IonCompiler", "VmSpecific", and "Frontend", which are just subsets of individual items. You could have a "LowOverhead" category that you enable during your runs. Since Categories are expected to overlap with each other, putting an item in one category doesn't preclude you from also including it in another, or controlling it individually.

> - This feels like a lot of boilerplate for simple cases. Perhaps invoking
> the dark magic of macros and/or templates could trim this down?

If you're referring to the boilerplate in TraceLoggingMeasure.cpp, then I agree, it's too much boilerplate. I'll work on this.

  

  



    
    

    
  
> Does C++ have a portable syntax for specifying the field names when initializing like this?

It does not, as far as I can tell. GCC added a syntax for this as an extension; it was incorporated into ISO C99 but never C++.

Steve raised the legibility issue as well. I'm trying to rework this in that light.

  

  



    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
  

        Attachment #8919996 -
        Flags: review?(n.nethercote)

        Attachment #8919997 -
        Flags: review?(n.nethercote)

        Attachment #8919998 -
        Flags: review?(sphink)

        Attachment #8919999 -
        Flags: review?(sphink)

    
    

    
  
Comment on attachment 8919997 [details]
Bug 1369955: Introduce a perfstream::Measure subtree for SpiderMonkey.

https://reviewboard.mozilla.org/r/190952/#review196134


C/C++ static analysis found 3 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: js/src/vm/Measure.cpp:87
(Diff revision 1)
> +
> +// A group of toy measures, for use in tests.
> +// This is actually an impl class for use with StaticGroup.
> +class MeasureTestsImpl {
> +  public:
> +    MeasureTestsImpl() { }

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

    MeasureTestsImpl() { }
    ^
                       = default;

::: js/src/vm/Measure.cpp:88
(Diff revision 1)
> +// A group of toy measures, for use in tests.
> +// This is actually an impl class for use with StaticGroup.
> +class MeasureTestsImpl {
> +  public:
> +    MeasureTestsImpl() { }
> +    MeasureTestsImpl(const MeasureTestsImpl&) { }

Warning: Use '= default' to define a trivial copy constructor [clang-tidy: modernize-use-equals-default]

    MeasureTestsImpl(const MeasureTestsImpl&) { }
    ^
                                              = default;

::: js/src/vm/Measure.cpp:133
(Diff revision 1)
> +    enum Options {
> +        Testing = 1,
> +    };
> +
> +    JSMeasuresImpl(JSContext* cx, bool options) : cx(cx), options(options) { }
> +    JSMeasuresImpl(const JSMeasuresImpl& rhs) : cx(rhs.cx), options(rhs.options) { }

Warning: Use '= default' to define a trivial copy constructor [clang-tidy: modernize-use-equals-default]

    JSMeasuresImpl(const JSMeasuresImpl& rhs) : cx(rhs.cx), options(rhs.options) { }
    ^
                                                                                 = default;

  

  



    
    

    
  
Okay, this is heavily simplified from the previous version. It probably is best to review it de novo, rather than comparing it to the prior submission.

This version distinguishes between Measures, which are leaf nodes representing individual controls; and Groups, which have child Measures and sub-Groups. This trims down the abstract base classes.

There's no longer any inherent notion of setting or clearing entire groups or subtrees; if it's important, define a Measure that just sets everything you care about in one step.

The detailed metadata in the prior version has been mostly replaced with a free-form list; we can add to it as the need arises.

The StaticGroup template provides an abbreviated way of defining simple groups. It's used in the implementations of the JSMeasures, TraceLogger, and MeasureTests groups. Hopefully the boilerplate is tolerable now.

As njn suggested, measure and group identifiers can now be arbitrary UTF-8 strings; there is no reserved separator character (other than '\0', I guess). Paths are arrays of strings.

Like the original, this is designed to permit static representations of the tree, so that it can be available at startup without imposing initialization overhead.

  

  



    
    

    
  
(In reply to Static Analysis Bot [:clangbot] from comment #22)
> C/C++ static analysis found 3 defects in this patch.

❤️

  

  



    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8874079 -
        Attachment is obsolete: true

    
  

        Attachment #8874080 -
        Attachment is obsolete: true

    
  

        Attachment #8874081 -
        Attachment is obsolete: true

    
  

        Attachment #8874082 -
        Attachment is obsolete: true

        Attachment #8874082 -
        Flags: feedback?(sphink)

    
    

    
  
Comment on attachment 8919998 [details]
Bug 1369955: Implement a JavaScript `Measure` API to the perfstream::Measure tree.

https://reviewboard.mozilla.org/r/190954/#review196544

I think I'd like to see it again after this round of changes. There isn't anything I'm particularly worried about.

Hopefully clearing the review is the way to do that in mozreview.

Ugh. And mozreview is failing to publish with:

Error publishing: Bugzilla error: Application failed during request deserialization: 
not well-formed (invalid token) at line 73, column 6, byte 3015 at /data/www/bugzilla.mozilla.org/local/lib/perl5/x86_64-linux-thread-multi/XML/Parser.pm line 187.

::: js/src/builtin/MeasureObject.h:59
(Diff revision 2)
> +    enum {
> +        // A Private value pointing to the perfstream::Group this object refers
> +        // to. On MeasureGroup.prototype, this refers to the root measure, which
> +        // the MeasureGroup and Measure constructors consult for path lookups.
> +        GroupSlot,
> +
> +        // Measure.prototype, for constructing child Measure instances.
> +        MeasureProtoSlot,
> +
> +        SlotCount

Hm. Most other users in the tree use static const uint32_t (or uint8_t, or unsigned). And it seems like someone complained when I attempted using enum, and the complaint was for a valid reason that I've now forgotten... perhaps because getReservedSlot() takes uint32_t? Doesn't that produce a warning if you pass in an enum?

::: js/src/builtin/MeasureObject.h:135
(Diff revision 2)
> +        // Use `isPrivate` and `isObject` to distinguish the two cases.
> +        MeasureOrRootSlot,

Let's see. I see a separate prototype class for ArrayIterator, StringIterator, TypedArray (& variants), DataView, Map, Set, Promise, Date... it really seems like the accepted setup is to use a different js::Class for the prototype. (On IRC you said that Date.prototype is an ordinary Object, but I don't think that's true -- they *stringify* using js_Object_str, but the Class is distinct, and protoobj->as<DateObject>() would fail IIUC.)

So unless you can show otherwise, I think it'd be better to stick to that scheme. Hopefully you can share pretty much everything else, even if you waste a slot or three on the prototype.

::: js/src/builtin/MeasureObject.cpp:37
(Diff revision 2)
> +using mozilla::perfstream::Description;
> +using mozilla::perfstream::Group;
> +using mozilla::perfstream::GroupPtr;
> +using mozilla::perfstream::GroupVector;
> +using mozilla::perfstream::Measure;
> +using mozilla::perfstream::MeasurePtr;
> +using mozilla::perfstream::MeasureVector;

Seems like 'using mozilla::perfstream' would be forgivable here, but if you prefer the explicit imports that's fine.

::: js/src/builtin/MeasureObject.cpp:62
(Diff revision 2)
> +    if (!str)
> +        return nullptr;
> +    return UniqueFreePtr<char>(JS_EncodeStringToUTF8(cx, str));
> +}
> +
> +static bool

This comment is actually for ToUTF8's return value, but I'm trying to convince mozreview to publish review and I'm wondering if it's getting confused by the Ctrl-Ls? (I'm looking for something like bug 1371595).

Why UniqueFreePtr rather than JS::UniqueChars? (from js/public/Utility.h)

::: js/src/builtin/MeasureObject.cpp:90
(Diff revision 2)
> +    return false;
> +}
> +
> +/*
> + * Check the type of a path argument to a Measure or MeasureGroup constructor,
> + * and retrieve its elements. On success, return a ValueVector holding the



::: js/src/builtin/MeasureObject.cpp:90
(Diff revision 2)
> +    return false;
> +}
> +
> +/*
> + * Check the type of a path argument to a Measure or MeasureGroup constructor,
> + * and retrieve its elements. On success, return a ValueVector holding the

Doesn't return a ValueVector. Not anymore, anyway.

::: js/src/builtin/MeasureObject.cpp:115
(Diff revision 2)
> +
> +    Rooted<ValueVector> pathValues(cx, ValueVector(cx));
> +    if (!pathValues.resize(length) || !GetElements(cx, pathArray, length, pathValues.begin()))
> +        return Nothing();
> +
> +    Vector<UniqueFreePtr<char>> pathUTF8(cx);

Could this be a js::Vector<UniqueChars> instead? js::Vector defaults to using TempAllocPolicy, which would do oom reporting for you (matching ValueVector).

::: js/src/builtin/MeasureObject.cpp:121
(Diff revision 2)
> +    if (!pathUTF8.reserve(pathValues.length())) {
> +        JS_ReportOutOfMemory(cx);
> +        return Nothing();
> +    }
> +
> +    for (size_t i = 0; i < pathValues.length(); i++) {

I *think* this should work:

    for (auto pathElement : pathValues) {
        UniqueSomething utf8 = ToUTF8(cx, pathElement);
        ...
    }

::: js/src/builtin/MeasureObject.cpp:146
(Diff revision 2)
> +                Vector<UniqueFreePtr<char>>& path, size_t length,
> +                HandleValue pathValue)
> +{
> +    GroupPtr here = start.Clone();
> +
> +    for (size_t i = 0; i < length; i++) {

I think this one can use c++ iteration too.

::: js/src/builtin/MeasureObject.cpp:156
(Diff revision 2)
> +        }
> +
> +        here = Move(*next);
> +    }
> +
> +    return Move(here);

Hm. I'm shaky on this stuff, but does this Move() do anything?

::: js/src/builtin/MeasureObject.cpp:187
(Diff revision 2)
> +
> +static bool
> +CommonToString(JSContext* cx, const char* className, const Description& description,
> +               MutableHandleValue result)
> +{
> +    UniqueChars cString = JS_smprintf("[object %s %s]", className, description.mIdentifier);

Doesn't this require a null check?

::: js/src/builtin/MeasureObject.cpp:195
(Diff revision 2)
> +    MeasureArgs(JSContext* cx, unsigned argc, Value* vp, const char *fnName)
> +      : cx_(cx),
> +        args(CallArgsFromVp(argc, vp)),

I know it's a pain and you intended this to be a CallArgs-like thing, but I'd really rather only see argc+vp in the toplevel JSNatives, even though it means repeating the CallArgsFromVp in each one.

::: js/src/builtin/MeasureObject.cpp:228
(Diff revision 2)
> +    Rooted<Object*> object_;
> +    RootedObject prototype_;

Both should be RootedObject, sadly.

::: js/src/builtin/MeasureObject.cpp:243
(Diff revision 2)
> +            JS_ReportErrorNumberASCII(cx_, GetErrorMessage, nullptr, JSMSG_NOT_NONNULL_OBJECT,
> +                                      InformalValueTypeName(thisValue));
> +            return false;
> +        }
> +
> +        JSObject* thisObject = &thisValue.toObject();

JSObject& seems more consistent with what you've done elsewhere.

::: js/src/builtin/MeasureObject.cpp:251
(Diff revision 2)
> +                                      Object::class_.name, fnName,
> +                                      thisObject->getClass()->name);
> +            return false;
> +        }
> +
> +        object_ = &thisObject->as<Object>();

I'm finding this 'Object' thing very confusing. "What namespace are we in? Is this some sort of perfstream::Object?" ObjectT would be easier for me, at least.

::: js/src/builtin/MeasureObject.cpp:261
(Diff revision 2)
> +                                      Object::class_.name, fnName, "prototype object");
> +            return false;
> +        }
> +
> +        // Object is never a proxy class, so this should always succeed.
> +        MOZ_ASSERT(GetPrototype(cx_, object_, &prototype_));

Whoa, this will be compiled out in an opt build. I think you meant MOZ_ALWAYS_TRUE.

::: js/src/builtin/MeasureObject.cpp:395
(Diff revision 2)
> +    Rooted<MeasureGroupObject*> proto(cx, &protoVal.toObject().as<MeasureGroupObject>());
> +    const Group& root = proto->measureGroup();
> +
> +    // The no-argument case: construct a MeasureGroupObject referring to the
> +    // root of the measure tree.
> +    if (args.length() == 0) {

I thought that things had now moved towards treating undefined as nonexistent, so that this would be

    if (args.get(0).isUndefined())

::: js/src/builtin/MeasureObject.cpp:463
(Diff revision 2)
> +
> +    Rooted<ValueVector> values(cx, ValueVector(cx));
> +    if (!values.reserve(measures.length()))
> +        return false;
> +
> +    for (size_t i = 0; i < measures.length(); i++) {

Does c++11 iteration work here?

::: js/src/builtin/MeasureObject.cpp:495
(Diff revision 2)
> +
> +    Rooted<ValueVector> values(cx, ValueVector(cx));
> +    if (!values.reserve(subgroups.length()))
> +        return false;
> +
> +    for (size_t i = 0; i < subgroups.length(); i++) {

iteration? (I should probably just figure it out myself, so I don't have to take back all of these comments. I'll stop commenting on them now.)

::: js/src/builtin/MeasureObject.cpp:575
(Diff revision 2)
> +    assertSameCompartment(cx, proto);
> +
> +    JSObject *plainObj = NewObjectWithGivenProto(cx, &class_, proto);
> +    if (!plainObj)
> +        return nullptr;
> +    Rooted<MeasureObject*> measureObj(cx, static_cast<MeasureObject*>(plainObj));

Can't this be

    Rooted<...> measureObj(cx, &plainObj->as<MeasureObject>());
    
like you've done elsewhere?

::: js/src/builtin/MeasureObject.cpp:690
(Diff revision 2)
> +    if (ToBoolean(args[0])) {
> +        measure.Enable();
> +    } else {
> +        measure.Disable();
> +    }

You're in spidermonkeyland. Lose the braces.

::: js/src/jit-test/tests/measures/MeasureGroup.js:22
(Diff revision 2)
> +  for (measure of measures) {
> +    let byPath = Measure(path.concat([measure.identifier]));

Seems like this should be

    for (const measure of measures) {
        const byPath = ...;
    }

::: js/src/jit-test/tests/measures/MeasureGroup.js:61
(Diff revision 2)
> +assertEq(tests.subgroups().length, 0);
> +
> +var testMeasures = tests.measures();
> +assertEq(testMeasures.length, 3);
> +assertEq(testMeasures.every(elt => {
> +  return ['Flopsy', 'Mopsy', 'Cottontail'].indexOf(elt.identifier) >= 0;

/me prefers arr.includes(elt.identifier)

::: js/src/shell/js.cpp:7949
(Diff revision 2)
>                  return nullptr;
>              if (!DefineConsole(cx, glob))
>                  return nullptr;
>          }
>  
> +

I think there's an extra blank line here.

::: js/src/shell/moz.build:44
(Diff revision 2)
>  if CONFIG['ENABLE_INTL_API'] and CONFIG['MOZ_ICU_DATA_ARCHIVE']:
>      # The ICU libraries linked into libmozjs will not include the ICU data,
>      # so link it directly.
>      USE_LIBS += ['icudata']
>  
> +USE_LIBS += ['perfstream']

Wait, what? What is this separate perfstream library thing?

  

  



        Attachment #8919998 -
        Flags: review?(sphink)

    
    

    
  
Comment on attachment 8919999 [details]
Bug 1369955: Add perfstream::Measure subtree for SpiderMonkey's TraceLogger.

https://reviewboard.mozilla.org/r/190956/#review196618

Wow, this came out looking pretty nice. Thank you!

::: js/src/vm/TraceLoggingMeasure.cpp:36
(Diff revision 2)
> +using mozilla::perfstream::StaticGroup;
> +using mozilla::Some;
> +
> +namespace js {
> +
> +class TraceLoggerMeasure: public Measure {

I think house style has a space before the colon.

::: js/src/vm/TraceLoggingMeasure.cpp:49
(Diff revision 2)
> +    TraceLoggerMeasure(JSContext* cx, TraceLoggerTextId textId) : cx(cx), textId(textId) { }
> +    TraceLoggerMeasure(const TraceLoggerMeasure& rhs) : cx(rhs.cx), textId(rhs.textId) { }
> +    TraceLoggerMeasure(TraceLoggerMeasure&& rhs) : cx(rhs.cx), textId(rhs.textId) { }
> +
> +    bool Enable() override {
> +        check_context([this]() { TraceLogEnableTextId (cx, textId); });

Extra space before (cx

  

  



        Attachment #8919999 -
        Flags: review?(sphink) → review+

    
    

    
  
sfink: I apologize for all the style violations; thanks very much for the close review. I think I'd gone a little blind from looking at the code for too long. I'll get a revision up soon.

  

  



    
    

    
  
jimb: should I wait for a new version? Realistically, I wasn't going to get to this until Monday anyway...

  

  



    
    

    
  
<jimb> sfink: For MeasureGroup, the prototype is actually a usable MeasureGroup instance; there's nothing special about it, except that the group it refers to is the root for lookups using that constructor. If I were to give it its own Class, it would be identical to the MeasureGroup class. (The same is not true of Measure, where there's a clearer distinction between prototype and instance.) Is it okay if I only break out Measure.prototype into its own Class, and leave MeasureGroup.prototype and MeasureGroup instances using the same Class?

<sfink> jimb: ok, with that info, I'm fine with the enum.

  

  



    
    

    
  
(In reply to Nicholas Nethercote [:njn] from comment #31)
> jimb: should I wait for a new version? Realistically, I wasn't going to get
> to this until Monday anyway...

I don't think you need to block on my addressing Steve's comments. The patches I asked him to review follow yours in the series, and his comments don't seem to ask for anything that would merit changes to lower levels.

(Monday will be fine, of course.)

  

  



    
    

    
  
This is the line I meant to quote:

<sfink> jimb: and wrt the prototype, I think I'll ask waldo before I do the next round of review, so for now at least "yes, that's fine"

  

  



    
    

    
  
In some cases, I can use the `for (var : iterable)` syntax, but not all. In particular, `ValueVector` can be indexed to produce a `HandleValue`, but `for (:)` doesn't seem to pick up on that. Using `for (auto& var : iterable)` works for vectors of unique pointers, so I've changed those.

  

  



    
    

    
  
> ::: js/src/builtin/MeasureObject.cpp:228
>(Diff revision 2)
> > +    Rooted<Object*> object_;
> > +    RootedObject prototype_;
>
> Both should be RootedObject, sadly.

Why is that? I'd like object_ to be the MeasureGroupObject or MeasureObject subclasses of JSObject, depending on the template parameter. Why doesn't this work?

  

  



    
    

    
  
> Wait, what? What is this separate perfstream library thing?

It's defined in earlier patches in the series, that njn is reviewing. It's the new top-level library that defines perfstream::Measure and perfstream::Group. It has a straight C++ API, so any module in the tree can contribute to it.

  

  



    
    

    
  
Splitting out Measure.prototype into its own class was a big win for simplicity. I didn't catch the signs that it needed to be its own type. Things are much more static now.

  

  



    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8919998 [details]
Bug 1369955: Implement a JavaScript `Measure` API to the perfstream::Measure tree.

https://reviewboard.mozilla.org/r/190954/#review196544

> Hm. Most other users in the tree use static const uint32_t (or uint8_t, or unsigned). And it seems like someone complained when I attempted using enum, and the complaint was for a valid reason that I've now forgotten... perhaps because getReservedSlot() takes uint32_t? Doesn't that produce a warning if you pass in an enum?

In discussion on IRC, we decided the enums were tentatively okay.

> Let's see. I see a separate prototype class for ArrayIterator, StringIterator, TypedArray (& variants), DataView, Map, Set, Promise, Date... it really seems like the accepted setup is to use a different js::Class for the prototype. (On IRC you said that Date.prototype is an ordinary Object, but I don't think that's true -- they *stringify* using js_Object_str, but the Class is distinct, and protoobj->as<DateObject>() would fail IIUC.)
> 
> So unless you can show otherwise, I think it'd be better to stick to that scheme. Hopefully you can share pretty much everything else, even if you waste a slot or three on the prototype.

I've separated out Measure.prototype into its own class; that was a big improvement. MeasureGroup.prototype is a fully-fledged MeasureGroup object in its own right, so it doesn't make sense to give it its own class.

> This comment is actually for ToUTF8's return value, but I'm trying to convince mozreview to publish review and I'm wondering if it's getting confused by the Ctrl-Ls? (I'm looking for something like bug 1371595).
> 
> Why UniqueFreePtr rather than JS::UniqueChars? (from js/public/Utility.h)

Changed the file to use JS::UniqueChars everywhere.

> I *think* this should work:
> 
>     for (auto pathElement : pathValues) {
>         UniqueSomething utf8 = ToUTF8(cx, pathElement);
>         ...
>     }

It doesn't, because C++ iteration won't give you HandleValues referring to the elements.

> I think this one can use c++ iteration too.

No, it doesn't always iterate over the whole vector. Commented.

> Hm. I'm shaky on this stuff, but does this Move() do anything?

It doesn't. Return values get mandatory copy elision. Removed, thanks.
http://en.cppreference.com/w/cpp/language/copy_elision

> Both should be RootedObject, sadly.

I would like these to each be the more specific JSObject subclasses. (See the latest iteration of the patch.) Does that not work?

> I'm finding this 'Object' thing very confusing. "What namespace are we in? Is this some sort of perfstream::Object?" ObjectT would be easier for me, at least.

Renamed to ThisObject; is that any better?

> Can't this be
> 
>     Rooted<...> measureObj(cx, &plainObj->as<MeasureObject>());
>     
> like you've done elsewhere?

I don't know what I was thinking with the `static_cast` garbage.

> Wait, what? What is this separate perfstream library thing?

See prior bugzilla comment.

  

  



    
    

    
  
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a060e917e558fc651739749f658183f2de7fb53

  

  



    
    

    
  
Comment on attachment 8919996 [details]
Bug 1369955: Add perfstream module. Define perfstream::Measure and Group types, with StaticGroup helper.

https://reviewboard.mozilla.org/r/190950/#review197080

My main comment about this is that it's all so abstract I'm still having trouble understanding it. In particular, what a "measure" is. At first I thought a measure was a single named measurement, the kind of thing that might also have a unit associated with it. But it seems like a measure is really something much bigger and more complicated than that; it's more like an entire profiling component or sub-component. I.e. TraceLogger's Baseline-measuring component is a measure.

So this code is a way of grouping such measures, naming them, enabling/disabling them, and providing some very simple documentation (e.g. 1-line descriptions) for them. And... that's it? You mention making "instrumentation immediately controllable via environment variables, GUIs for interactive users, chrome JavaScript APIs for addons, and so on", but I don't think any code of that form is actually present yet, right? And is there any facility for extracting data from measures? I don't think I saw any, but that seems kinda important.

What are the existing profiling components that we have that could be measures? TraceLogger is one. I guess the Gecko Profiler is another? What else? More examples in the comments would be highly illuminating.

Ultimately I'm still unclear about how all this code is going to be used. I appreciate that a lot of thought has gone into making this very general. But the abstractness, combined with the lack of examples and concrete use cases makes me fear that this is creeping into architecture astronaut territory. Just one example: we have trees of measures. Do we really need trees? How many measures will we actually have in practice? Would a list of measures suffice? Then all the stuff about groups and sub-groups could be removed.

It's entirely possible that I'm missing something important about all this.

----

On more minor points, I'll repeating some comments from my last review.

- I think this should all go under tools/.

- You've used some SpiderMonkey style-isms throughout. First, Gecko classes/enums/inline functions all have their braces on the following line. Second, Gecko if/else blocks are always braced. (You can use `./mach clang-format` to fix up the whitespace issues.)

::: perfstream/Measure.h:15
(Diff revision 1)
> + *
> + * - If you are implementing some sort of instrumentation, including your
> + *   measurements in this registry makes your instrumentation immediately
> + *   controllable via environment variables, GUIs for interactive users, chrome
> + *   JavaScript APIs for addons, and so on. With help messages and pop-up text.
> + *   Silver-platter service with minimal effort.

Nit: "Silver platter".

::: perfstream/Measure.h:18
(Diff revision 1)
> + *   controllable via environment variables, GUIs for interactive users, chrome
> + *   JavaScript APIs for addons, and so on. With help messages and pop-up text.
> + *   Silver-platter service with minimal effort.
> + *
> + * - If you would like to control the collection of performance data, this tree
> + *   aspires to be the central clearinghouse for enumerating and controlling data

Nit: "clearing house".

::: perfstream/Measure.h:42
(Diff revision 1)
> + * If the underlying data source goes away (the JavaScript runtime gets freed,
> + * for example), Measures or Groups referring to it are still safe to use;
> + * operations fail in a reported, recoverable way.
> + *
> + * The tree can change as the program runs. For example, there might be a Group
> + * named 'Worker Threads' with a subgroup for each live worker thread.

Right, this is a tree -- Measures are leaves and groups are non-leaves, right? -- but the terminology above avoids referring to it as a tree. I wonder if using tree terminology would be clearer.

::: perfstream/Measure.h:49
(Diff revision 1)
> + * How one actually obtains the root Group depends on the application.
> + *
> + * Goals:
> + *
> + * - Encourage developers to add performance data sources by providing an
> + *   off-the-shelf controller. Simply adding your data source to this tree makes

What does "controller" mean here?

::: perfstream/Measure.h:89
(Diff revision 1)
> +
> +/**
> + * Identifier, description, etc. for a Measure or Group.
> + *
> + * All strings are null-terminated UTF-8. They are borrowed from the Measure or
> + * Group. they must live at least as long as it does, but not longer.

Nit: "They"

I find this sentence hard to parse. What does "it" refer to -- the Measure or Group? Or the Description?

::: perfstream/Measure.h:97
(Diff revision 1)
> +  /**
> +   * The identifier for this measure/group, used in programmatic APIs like
> +   * environment variables or WebIDL interfaces. This is used as a component in
> +   * paths, like ["JavaScript", "IonMonkey"].
> +   *
> +   * This is never nullptr.

You could use mozilla::NotNull<> to make the non-nullness clearer. That type just got a MakeNotNull() function that makes it nicer to use.

::: perfstream/Measure.h:112
(Diff revision 1)
> +   *
> +   * For example:
> +   * ["tooltip.en-us: Time spent in the IonMonkey JavaScript compiler.",
> +   *  "tooltip-fluent: measure.spidermonkey.ionmonkey", // l20n identifier
> +   *  "help-url: https://wiki.mozilla.org/IonMonkey",
> +   *  nullptr]

If there's implicit structure within the strings, I wonder if it's worth representing that in the type itself. E.g. have something like this:

> struct Metadata {
>   const char* mKey;
>   const char* mValue;
> };

That way you won't have to do parsing to split up the key from the value, and you can't get malformed entries that are missing a colon.

::: perfstream/Measure.h:137
(Diff revision 1)
> + *
> + * This means that the API must hand out a fresh Measure object for each
> + * request, and in general there may be many Measure objects alive at a time
> + * that all refer to the same underlying component.
> + *
> + * The underlying component to which a Measure refers is called an "inner

I don't know what "component" means in this sentence. And that means the following two paragraphs are something of a mystery.

::: perfstream/Measure.h:214
(Diff revision 1)
> +  /**
> +   * Return a Description for this measure: its identifier and metadata.
> +   *
> +   * The strings returned are borrowed from this Measure; they last at least as
> +   * long as it does, but not necessarily any longer. If you need to hold on to
> +   * them, you should make copies.

The string handling in this entire API makes me nervous. Raw pointers and lots of comments about lifetimes, stuff that sounds easy to get wrong.

If this were Gecko-only code you could use nsCString and things would be a lot safe, but I guess you can't do that because of SpiderMonkey. Hmm. I wonder if making copies of strings more aggressively would make it less error-prone.

::: perfstream/Measure.h:236
(Diff revision 1)
> + *
> + * A Group represents an interior node in the tree, with Measures and other
> + * Groups as its children.
> + *
> + * The names of all a Group's children, measures and subgroups, must be
> + * distinct.

What happens if they are not distinct?

::: perfstream/Measure.cpp:38
(Diff revision 1)
> +  GroupPtr ownedHere;
> +
> +  /* Walk down through all the parent groups. */
> +  for (size_t i; i < aPathLength - 1; i++) {
> +    auto next = here->GetSubgroup(aPath[i]);
> +    if (!next)

Brace one-line blocks, please.

::: perfstream/Measure.cpp:51
(Diff revision 1)
> +}
> +
> +Maybe<GroupPtr>
> +Group::GetSubgroupByPath(const char** aPath, size_t aPathLength) const
> +{
> +  /*

Add an aPathLength assertion like the one in GetMeasureByPath()?

In general, these two functions have a lot of overlap. It would be nice to factor that out.

::: perfstream/Measure.cpp:61
(Diff revision 1)
> +  GroupPtr ownedHere;
> +
> +  for (size_t i; i < aPathLength; i++) {
> +    auto next = here->GetSubgroup(aPath[i]);
> +    if (!next)
> +      return Nothing();

Another one-line block.

::: perfstream/StaticGroup.h:70
(Diff revision 1)
> +                                          std::function<MeasurePtr()> constructor) {
> +        auto child = constructor();
> +        return child && children.append(Move(child));
> +      });
> +    if (!ok)
> +      return mozilla::Nothing();

Missing braces. (And more like that below.)

  

  



        Attachment #8919996 -
        Flags: review?(n.nethercote)

    
    

    
  
Comment on attachment 8919997 [details]
Bug 1369955: Introduce a perfstream::Measure subtree for SpiderMonkey.

https://reviewboard.mozilla.org/r/190952/#review197098

::: js/src/vm/Measure.cpp:39
(Diff revision 2)
> +
> +
> +namespace js {
> +
> +// A dummy measure, for testing only.
> +class TestMeasure: public Measure {

All this test code feels like something that should be in a separate test file, and not compiled into the product.

  

  



        Attachment #8919997 -
        Flags: review?(n.nethercote)

    
    

    
  
> My main comment about this is that it's all so abstract I'm still having trouble understanding it.

I just re-read comment 0 and I see it does a decent job of describing this. That comment, or similar words, would be useful to have in the comments in the code. (More examples of measures would still be helpful, though.)

Above I suggested that this design was too general, but now that I think some more I wonder if in other ways it might be not general enough. Specifically, Measure only has boolean Enable() and Disable() methods. But profiler components generally have lots of ways to start up: various flags, levels of details/verbosity, output filenames, etc. You might be able to capture some of that (flags) via sub-groups (with one Measure per flag) but getting the other stuff in there seems difficult.

  

  



    
    

    
  
(In reply to Jim Blandy :jimb from comment #43)
> > Both should be RootedObject, sadly.
> 
> I would like these to each be the more specific JSObject subclasses. (See
> the latest iteration of the patch.) Does that not work?

No, that's fine. (And better.) I was only referring to "RootedObject" vs "Rooted<JSObject*>". Spidermonkey style wants the former; Gecko style wants the latter. If you're going to have more than a few in Spidermonkey code, the style-compatible thing to do is define a RootedMeasureObject or whatever. But since I personally prefer the explicit Rooted<> usage in the first place, I promise to look the other way if you just use Rooted<MeasureObject*> everywhere. (But do use RootedObject in favor of Rooted<JSObject*>, for consistency. It looks like we're pretty good about it -- 1762 RootedObject vs 39 Rooted<JSObject*> right now.)

> > I'm finding this 'Object' thing very confusing. "What namespace are we in? Is this some sort of perfstream::Object?" ObjectT would be easier for me, at least.
> 
> Renamed to ThisObject; is that any better?

Good enough. I wish there were some convention for template parameters, other than the informationless T and U, but it seems there isn't.

  

  



    
    

    
  
Comment on attachment 8919997 [details]
Bug 1369955: Introduce a perfstream::Measure subtree for SpiderMonkey.

https://reviewboard.mozilla.org/r/190952/#review197098

> All this test code feels like something that should be in a separate test file, and not compiled into the product.

I was emulating js/src/builtin/TestingFunctions.cpp, which defines testing-only material that is compiled into the product, but visible only on request (via Components.utils.getJSTestingFunctions). The MeasureTests group is visible only when requested by calling GetMeasuresWithTests.

Is that okay, or should I use some other criteria to make this visible in testing configurations? I'm not sure what's best here, if TestingFunctions.cpp isn't doing it.

  

  



    
    

    
  
Comment on attachment 8919997 [details]
Bug 1369955: Introduce a perfstream::Measure subtree for SpiderMonkey.

https://reviewboard.mozilla.org/r/190952/#review197098

> I was emulating js/src/builtin/TestingFunctions.cpp, which defines testing-only material that is compiled into the product, but visible only on request (via Components.utils.getJSTestingFunctions). The MeasureTests group is visible only when requested by calling GetMeasuresWithTests.
> 
> Is that okay, or should I use some other criteria to make this visible in testing configurations? I'm not sure what's best here, if TestingFunctions.cpp isn't doing it.

The patches following this one add a JavaScript API for the measure tree, which eventually will be used by the developer tools. TestMeasures serves as a measure that is guaranteed to be present, for the tests of that JavaScript API to manipulate. Since we want to ship our users the same bits we tested, I think this needs to ship.

  

  



    
    

    
  
Comment on attachment 8919998 [details]
Bug 1369955: Implement a JavaScript `Measure` API to the perfstream::Measure tree.

https://reviewboard.mozilla.org/r/190954/#review197334

::: js/src/builtin/MeasureObject.cpp:197
(Diff revision 3)
> +
> +// Generic |this| and argument checking for MeasureObject, MeasureGroupObject.
> +template<typename ThisObject>
> +class MeasureArgs MOZ_STACK_CLASS {
> +  public:
> +    MeasureArgs(JSContext* cx, CallArgs args, const char *fnName)

I see most users passing these as const CallArgs&. And I was about to make an argument for why this would be necessary, to enable the usedRval() tracking, but I notice that it isn't mutable and so the const invalidates that argument.

  

  



        Attachment #8919998 -
        Flags: review?(sphink) → review+

    
    

    
  
On IRC, we decided that CallArgs really does need to be passed by reference. Revised patch coming.

  

  



    
    

    
  
(In reply to Steve Fink [:sfink] [:s:] from comment #51)
> I see most users passing these as const CallArgs&. And I was about to make
> an argument for why this would be necessary, to enable the usedRval()
> tracking, but I notice that it isn't mutable and so the const invalidates
> that argument.

For the record, it *is* mutable, and I am blind.

  

  



    
    

    
  
Here's a new wrinkle:

The Group and Measure abstract base classes require implementations to return mozilla::UniquePtrs and mozilla::Vectors. SpiderMonkey cannot construct such values, as they use the vanilla operator new and delete, not js_new and js_delete.

Changing Group and Measure to use js::UniquePtr is no solution, as the intent is to return objects that code outside SpiderMonkey can own, and js::UniquePtr can't be used outside SpiderMonkey.

It's possible to specialize DefaultDeleter for Measure and Group to delegate deletion to a virtual method of Group and Method. This takes care of mozilla::UniquePtr<...>, but doesn't handle mozilla::Vector's element array allocations.

I am stuck on this for the moment.

  

  



    
    

    
  
I guess it could return mozilla::Vector with an AllocPolicy that holds a pointer to some object with virtual functions for the policy methods. Kafkaesque.

  

  



    
    

    
  
StaticGroup also uses std::function, which relies on operator new internally.

  

  



    
    

    
  
After some discussion on IRC with froydnj and Waldo, I've implemented a DynamicAllocPolicy for MFBT. I'll attach it to a new blocker bug once I have everything else sorted out.

  

  



    
    

    
  
Marking as P2 so it surfaces in my dashboards[1], although this is most likely a P3.

[1]: https://wiki.mozilla.org/Firefox/Performance_Tools_and_Outreach/Performance#Gecko_Profiler

  

  


Priority: -- → P2

    
    

    
      
  

    
    

    
  
Sorry, there was a problem with the detection of inactive users. I'm reverting the change.



  

  


Assignee: nobody → jimb
Status: NEW → ASSIGNED
Flags: needinfo?(gsquelart)

    
  
Severity: normal → S3

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: