Closed Bug 887027 Opened 11 years ago Closed 11 years ago

Implement a tracing profiler actor

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect, P2)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 25

People

(Reporter: rjacob, Assigned: rjacob)

References

Details

Attachments

(2 files, 11 obsolete files)

9.69 KB, text/plain
rcampbell
: feedback+
Details
54.24 KB, patch
past
: review+
rcampbell
: review+
Details | Diff | Splinter Review
This is the server side half of implementing a tracing profiler.
Assignee: nobody → rbailey
Attachment #767543 - Flags: feedback?(dcamp)
The protocol is designed so that you don't need to finish a profile before getting data on it, so that it's possible to provide a live view of the profile as it's collected.

The reason we use so many CallActors is so that a client can request information about many of them concurrently.
This is very interesting! It fixes my main gripes with the tracing profiler's protocol, but perhaps by going to the opposite side of the solution space :-) Here are some thoughts and questions that popped up while reading it:

How will the client keep its list of call actors up to date while the profiler is still working? Is it expected to poll the profileActor with repeated "calls" requests? Or maybe is the plan to just display whatever tree arrived in the first request and then make it look like everything else is collapsed/hidden (and require user action to expand them thereby making a new "calls" request)?

Since call arguments are known from the construction of the call actor, why not just include them in the call actor's form?

Getting the current time interval from a call actor seems to only be useful if the front-end is continually updating the view, but that would be costly in terms of protocol requests. Does it really make sense from the user's POV to display the current elapsed time or can the "return" and "time" requests be combined?

What is the benefit of having the client poll the call actor with "finished" requests and handling "notFinished" errors, instead of making the call actor send an unsolicited notification with the "return" and "time" fields when done? Using such an event could also have the call actor be automatically GC'd, reducing memory pressure on the server.

I suppose if the answers to the last three questions combined are positive, then there is not much benefit to keeping a call actor entity around, compared to a richer initial call form and unsolicited notifications from the profile actor when a call ends.
Summary: [meta] Implement a tracing profiler actor → Implement a tracing profiler actor
Attached file Revised tracing profiler protocol (obsolete) —
You have the right idea, Panos--the plan was to have the client repeatedly poll the profileActor with "calls" requests to get the call trees while the profile was being collected (after profiling ends, the profileActor's call tree list would be immutable). The client could then request more information from call actors when they are expanded or otherwise visible in the view. The benefit of having callActors instead of sending all call information with the call trees would be to minimize the size of the "calls" response, and allow concurrent requests to callActors when calls need to be rendered in the view for the first time.

That approach requires the server to keep the entire call tree in memory and send it over the wire with every "calls" request. This would start to be a problem pretty quickly; call trees would become very large since calls are recorded per function call rather than per call site.

This revised protocol spec sends a stream of unsolicited packets on calls and returns. It's then the client's job to construct call trees or aggregate data as needed.
Attachment #767543 - Attachment is obsolete: true
Attachment #767543 - Flags: feedback?(dcamp)
Attachment #767977 - Flags: feedback?(past)
Attachment #767977 - Flags: feedback?(dcamp)
console.profile(End) also accepts an option argument: name of a profile so your startedTracing event should send that info as well. With SPS we just pass the name to the client where we keep track of all the running profiles.
s/option/optional
Is

  console.profile("foo");
  console.profile("bar");
  console.profileEnd("foo");
  console.profileEnd("bar");

valid?

Do we need to support concurrent profiles?

Do we need to support them and assume that they aren't started and ended in a LIFO order?
(In reply to Nick Fitzgerald [:fitzgen] from comment #7)
> Is
> 
>   console.profile("foo");
>   console.profile("bar");
>   console.profileEnd("foo");
>   console.profileEnd("bar");
> 
> valid?

Yep.

> Do we need to support concurrent profiles?

Yep, that's how we do with SPS and that's how Chrome does with their profile(End) implementation.

> Do we need to support them and assume that they aren't started and ended in
> a LIFO order?

Yes. If profileEnd is called with an argument then it either stops only that profile or does nothing (if profile doesn't exist). If it is called without any arguments it simply pops the most recently started profile and stops it. Keep in mind that it shouldn't distinguish between profiles started via console.profile and profiles started via the GUI when called without arguments.
Comment on attachment 767977 [details]
Revised tracing profiler protocol

I believe this is much better, thanks! My only serious concern is whether the enteredFrame/exitedFrame notifications can be used to properly construct a call tree. Since timestamps are optional, are we only relying on the order of the transmitted notifications here? Actually, why is recording the time optional? Do we plan to use this backend to also implement a code coverage tool?

I suppose that we make arguments and return values optional to cut down on the imposed overhead, but if initial prototype testing doesn't indicate an order of magnitude improvement, I would just collect all the data I could get my hands on. I have a hunch that it *will* be a lot faster, especially on mobile devices, but I'm just mentioning this as a thing to keep in mind.

Anton made a great point about sending the profile name. One nit I have is that in the context of the tracing actor startedTracing and stoppedTracing are a bit verbose. started/finished should probably be enough.

Callsites should probably just use the existing source location definitions in the remote protocol:

https://wiki.mozilla.org/Remote_Debugging_Protocol#Source_Locations
Attachment #767977 - Flags: feedback?(past) → feedback+
We are relying on the order of the transmitted notifications, but doesn't the protocol guarantee that we can? Recording the time is optional because there are some cases where it wouldn't be needed (code coverage, call hierarchy/graphs, type reconstruction), and we figured we might as well not collect data if it's not requested.
(In reply to Panos Astithas [:past] from comment #9)
> Comment on attachment 767977 [details]
> Revised tracing profiler protocol
> 
> I believe this is much better, thanks! My only serious concern is whether
> the enteredFrame/exitedFrame notifications can be used to properly construct
> a call tree. Since timestamps are optional, are we only relying on the order
> of the transmitted notifications here? Actually, why is recording the time
> optional? Do we plan to use this backend to also implement a code coverage
> tool?

Order is enough to construct the tree.

Make everything optional! I think optional should be the default as long as it doesn't affect speed. Since you specify from the start what you want traced, we don't require any extra requests to get that info later on and so we won't introduce any more waiting.

> Anton made a great point about sending the profile name. One nit I have is
> that in the context of the tracing actor startedTracing and stoppedTracing
> are a bit verbose. started/finished should probably be enough.

+1

> Callsites should probably just use the existing source location definitions
> in the remote protocol:
> 
> https://wiki.mozilla.org/Remote_Debugging_Protocol#Source_Locations

+1
I'm on board with the optional data collection, but I think implicit ordering is not such a good idea.

Relying on the ordering imposed by the transport layer, although safe for now, is probably asking for trouble in the long run. Ideally we would like to be able to tweak the transport implementation in order to improve performance, so having implicit assumptions about it in the protocol messages would hinder that goal. We have done this in the past with the addition of LocalDebugggerTransport and we will be making more changes soon to accommodate binary data streams.

Maybe I'm overestimating the likelihood of making changes that break ordering guarantees to the protocol's transport layer, so if Jim or Dave feel that this is an OK assumption to make, I won't mind. But it does seem like a premature optimization to avoid sticking a cheap, ordinal property to the packet, if the client is going to depend on it.
Attached file Revised tracing profiler protocol (obsolete) —
I see your point! I added sequence numbers to this revision. I also added support for named traces and re-entrant startTrace requests, removed the "traceTypes" request, and added some documentation.
Attachment #767977 - Attachment is obsolete: true
Attachment #767977 - Flags: feedback?(dcamp)
Attachment #770386 - Flags: feedback?(past)
Attachment #770386 - Flags: feedback?(dcamp)
It seems like the "attach" request is missing; there are only responses and error responses.

Also, should probably document "detach" request/response.
Comment on attachment 770386 [details]
Revised tracing profiler protocol

+1 on Nick's comments.

"onAttach" would be the implementation method handler for the "attach" request, right? At least that's the standard debugger actor nomenclature.

If I understand the re-entrancy of "startTrace" correctly, the following scenario could come up:

- { to: TraceActorID, type: "startTrace", name: "myTrace" trace: { time: false, arguments: false, return: false } }
- { to: TraceActorID, type: "startTrace", name: "myTrace", trace: { time: true, arguments: true, return: true } }
- the resulting trace will consist of some initial records with no extra properties and other records with all of them.

Isn't this going to result in a seemingly broken trace from the user's perspective? What is the benefit from this approach compared to denying the second startTrace request or making it replace and restart the first one?

These comments notwithstanding, this looks very good!
Attachment #770386 - Flags: feedback?(past) → feedback+
(In reply to Panos Astithas [:past] from comment #15)
> - { to: TraceActorID, type: "startTrace", name: "myTrace" trace: { time:
> false, arguments: false, return: false } }
> - { to: TraceActorID, type: "startTrace", name: "myTrace", trace: { time:
> true, arguments: true, return: true } }
> - the resulting trace will consist of some initial records with no extra
> properties and other records with all of them.
> 
> Isn't this going to result in a seemingly broken trace from the user's
> perspective? What is the benefit from this approach compared to denying the
> second startTrace request or making it replace and restart the first one?

You're right in thinking that the only effect of the second request on the state of the trace actor is that it will start recording the extra properties. The client is supposed to replace and restart the trace though: "If a trace with the given name is already being or has already been collected, the client is expected to replace it with a new trace."
Attached file Revised tracing profiler protocol (obsolete) —
Addresses Nick's comments.

View as formatted HTML: https://gist.github.com/rjbailey/8a0c44e284b5db800041
Attachment #770386 - Attachment is obsolete: true
Attachment #770386 - Flags: feedback?(dcamp)
Attachment #770840 - Flags: feedback?(past)
Attachment #770840 - Flags: feedback?(dcamp)
(In reply to Jake Bailey from comment #16)
> You're right in thinking that the only effect of the second request on the
> state of the trace actor is that it will start recording the extra
> properties. The client is supposed to replace and restart the trace though.

In that case why not completely discard the previous configuration and any previously collected records and start anew? IOW what is the point of computing the union of the configuration options if the end result is that we no longer care about the previous request?

Also, how is the union going to work if the user changes his mind and decides that doesn't actually want arguments, but definitely wants time? How can a previous option be canceled?

Speaking of options, it just occurred to me that we don't need to send the full dictionary with true/false values in the "trace" property, maybe not even in "traceTypes". The web console, for example, sends an array with the desired listeners and gets back an array of those that were actually started:

{ "to": "conn0.console28", "type": "startListeners",
  "listeners": [
    "PageError",
    "ConsoleAPI",
    "NetworkActivity",
    "FileActivity"
  ]
}

{ "from": "conn0.console28", "nativeConsoleAPI": true,  
  "startedListeners": [
    "PageError",
    "ConsoleAPI",
    "NetworkActivity",
    "FileActivity"
  ]
}
(In reply to Panos Astithas [:past] from comment #18)
> In that case why not completely discard the previous configuration and any
> previously collected records and start anew? IOW what is the point of
> computing the union of the configuration options if the end result is that
> we no longer care about the previous request?

The intent is to collect the union of the requested trace types for all *active* traces. If we start a trace named "foo" that collects time, and then a trace "bar" that collects arguments, then on frame entry we want to send both time and arguments. When "foo" is stopped we can stop collecting time, and when "bar" stops we stop collecting arguments. When receiving a frame entry packet, the client adds only the "time" value to the "foo" trace, and the "arguments" value to the "bar" trace.

If both traces were called "foo", then we would stop caring about time and only collect arguments once the second trace started. The client would throw away the first trace with the time values. I'm not sure how useful this really is, though. It might be simpler to send an error response when attempting to start a trace with the same name as an existing active trace.

> Speaking of options, it just occurred to me that we don't need to send the
> full dictionary with true/false values in the "trace" property, maybe not
> even in "traceTypes". The web console, for example, sends an array with the
> desired listeners and gets back an array of those that were actually started:

That makes more sense, yeah.
Ah, OK. So the union of configuration options applies only to separate traces, not replacement traces. Might want to clarify that somehow in the final spec document. Alternatively, treating this case as an error sounds fine to me, too.
Attachment #770840 - Flags: feedback?(past) → feedback+
I was under the impression that we could ensure that there wouldn't be any duplicate traces by prefixing all console.profile("foo") so that its profile name was "console.foo" and any profiles started by the client as "client.foo" and then we never have to worry about conflicting profile names.

I guess this wouldn't work with client implementations other than our own.
Requested by Nick.
Attachment #773998 - Flags: feedback?(nfitzgerald)
Priority: -- → P2
Comment on attachment 773998 [details] [diff] [review]
WIP patch for trace actor and stub trace client

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

Great job! This is looking really nice, I'm excited for this to land :)

::: toolkit/devtools/client/dbg-client.jsm
@@ +511,5 @@
> +        aOnResponse(aResponse, traceClient);
> +      }
> +    });
> +  },
> +

We need to call `aOnResponse` whether or not there is an error so that the callback can inspect the error message and decide how to proceed.

Also, I think we should use `DebuggerClient.requester` because it will take care of a lot of little details for us.

DebuggerClient.requester: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/client/dbg-client.jsm#242

Example use (although I don't think you will need to use `before` or `after`): http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/client/dbg-client.jsm#1162

And you will need to add two new telemetry probes, a remote and local version, like this: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Histograms.json#3381

@@ +1592,5 @@
> +  /**
> +   * Detach from the trace actor.
> +   */
> +  detach: DebuggerClient.requester({ type: "detach" },
> +                                   { telemetry: "DETACH" }),

We need to create two new telemetry probes for this type of detach, otherwise we would be conflating the stats for different types of requests. See the mxr link to Histograms.json above.

@@ +1594,5 @@
> +   */
> +  detach: DebuggerClient.requester({ type: "detach" },
> +                                   { telemetry: "DETACH" }),
> +
> +  startTrace: function TPC_startTrace(aTraceTypes, aName, aOnResponse) {

I think we can use `DebuggerClient.requester` here as well if you hook into it with `before` and `after` hooks.

@@ +1630,5 @@
> +      aOnResponse(aResponse);
> +    }.bind(this));
> +  },
> +
> +  stopTrace: function TPC_stopTrace(aName, aOnResponse) {

Again, I think we can use `DebuggerClient.requester` here as well.

::: toolkit/devtools/server/actors/tracer.js
@@ +113,5 @@
> +    while (this.tracing) {
> +      this.onStopTrace({});
> +    }
> +
> +    delete this.dbg;

Nit: we should do `this.dbg = null` because right now `delete` forces dictionary mode on objects in SpiderMonkey, while setting to `null` does not.

@@ +127,5 @@
> +      this._startTime = +new Date;
> +    }
> +
> +    // Start recording all requested trace types.
> +    for (let traceType of aRequest.trace) {

Should we fail when we get a request to trace an unknown trace type?

@@ +138,5 @@
> +    let nameIndex = this._activeTraces.lastIndexOf(aRequest.name);
> +    if (nameIndex >= 0) {
> +      this._activeTraces.splice(nameIndex, 1);
> +    }
> +    this._activeTraces.push(name);

It took me a couple minutes of reading code here and elsewhere to figure out why we are forcing this to be the last trace (so that the next stop tracing request stops the right request, correct?)

I think it would be nice to have a comment here explaining why, or even abstract over the stack of traces and all of this splicing and last index stuff in this function and elsewhere.

@@ +178,5 @@
> +      sequence: this._sequence++
> +    };
> +
> +    if (callee && callee.parameterNames.length) {
> +      packet.parameterNames = callee.parameterNames;

Do you think this should be a trace type? When there aren't any parameters, I think we should send an empty array of parameter names rather than just leave out the property. This lets the client always iterate over the parameter names without having to check if they exist or not, which I imagine could be a little paper cut annoyance.

@@ +184,5 @@
> +
> +    if (aFrame.script) { // When is this not true?
> +      packet.callsite = {
> +        url: aFrame.script.url,
> +        line: aFrame.script.getOffsetLine(aFrame.offset)

We should add columns here too. I have a patch in review which adds this to the debugger. In the mean time you can copy this function:

https://bugzilla.mozilla.org/attachment.cgi?id=772411&action=diff#a/toolkit/devtools/server/actors/script.js_sec16

But we should really have a follow up bug once your patch lands to share code between the script actors and the tracer actor.

@@ +199,5 @@
> +      for (let i in aFrame.arguments) {
> +        packet.arguments.push(aFrame.arguments[i]);
> +      }
> +    }
> +

Do you think it makes sense to abstract out the trace types so that they aren't hard coded in entering and exiting frames? Instead we might have some API like

  TraceTypes.registerTraceType("arguments", TraceTypes.ON_ENTER_FRAME, function (frame) {
    return [serialize(arg) for (let arg of frame.arguments)];
  });

and then here in `onEnterFrame` we'd do something like:

  for (let [name, tracer] of TraceTypes.iterEnterFrameTracers()) {
    packet[name] = tracer(frame);
  }

and there would be similar things for exiting frames.

Not sure if this is over-engineering for the problem but seeing all these repeated checks on whether we are tracing a certain trace type makes me slightly suspicious.

::: toolkit/devtools/server/tests/unit/test_trace_actor-02.js
@@ +48,5 @@
> +}
> +
> +function start_named_trace(aName)
> +{
> +  let deferred = Promise.defer();

As mentioned in the meeting today, we are moving to little 'p' promises. I think you will need to change this next time you rebase on fx-team.
Attachment #773998 - Flags: feedback?(nfitzgerald) → feedback+
Attached file Tracing profiler protocol (obsolete) —
The gist for this file has been revised (and therefore has a diff against the previous version): https://gist.github.com/rjbailey/8a0c44e284b5db800041

The major addition to this version is a specification for how objects in arguments and return values are to be serialized. The serialized values are similar to the values used by object actors, only instead of object grips, indices into an object pool (also sent in the enteredFrame/exitedFrame response) are used. This avoids issues with circular references and relieves the server from needing to keep copies of every argument and return value in memory.
Attachment #770840 - Attachment is obsolete: true
Attachment #770840 - Flags: feedback?(dcamp)
Attachment #777506 - Flags: feedback?(past)
Attachment #777506 - Flags: feedback?(dcamp)
Minor changes to serialization spec. Adds more properties to object descriptors and makes grips more consistent with ObjectActor value grips.
Attachment #777506 - Attachment is obsolete: true
Attachment #777506 - Flags: feedback?(past)
Attachment #777506 - Flags: feedback?(dcamp)
Attachment #778719 - Flags: feedback?(dcamp)
I'm having trouble with try, so I'll need someone else to do a try push until I can get everything figured out.
Attachment #773998 - Attachment is obsolete: true
Attachment #778730 - Flags: review?(past)
Argh, trychooser hasn't been updated with the new syntax for B2G builds. Additional push:
https://tbpl.mozilla.org/?tree=Try&rev=e6f9f6ea6bd0
Comment on attachment 778730 [details] [diff] [review]
Create trace actor and stub trace client

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

Nice job!

::: toolkit/devtools/client/dbg-client.jsm
@@ +1658,5 @@
> +  }, {
> +    before: function(aPacket) {
> +      if (!aPacket.name) {
> +        aPacket.name = undefined;
> +      }

Do we really need to do this? Seems like overkill to me, I think we could just skip using the `before` hook here.

::: toolkit/devtools/server/actors/tracer.js
@@ +46,5 @@
> +   */
> +  _handleEvent: function(aEvent, aPacket, ...args) {
> +    let handlersForEvent = TraceTypes.handlers[aEvent];
> +    for (let traceType in handlersForEvent) {
> +      if (this._requestsForTraceType[traceType] > 0) {

Nit: you can leave off the `> 0` if you want

@@ +110,5 @@
> +        aGlobal.hostAnnotations.type == "document" &&
> +        aGlobal.hostAnnotations.element === this.global) {
> +      this._addDebuggee(aGlobal);
> +    }
> +  },

I think we might want to reuse the concept of a `globalManager` from the debugger. Swapping out global managers allows us to create a chrome or addon-sdk tracer without changing the actual actor.

@@ +246,5 @@
> +   *
> +   * @param aFrame Debugger.frame
> +   *        The stack frame that was entered.
> +   */
> +  onExitFrame: function(aValue) {

Nit: documentation says aFrame, function says aValue; they should be consistent with each other.

@@ +284,5 @@
> + * ordering. In addition to the push and pop stack operations, supports a
> + * "remove" operation, which removes the object with a given name from any
> + * location in the stack.
> + */
> +function NamedObjectStack()

Nit: isn't this more of a `SetStack` since each item is supposed to be unique? Personally, I'd like that name better but I can live with `NamedObjectStack` if other people disagree with me.

@@ +440,5 @@
> +    }
> +  }
> +
> +  if (!bestOffsetMapping) {
> +    // XXX: Try not to completely break the experience of using the debugger for

Nit: s/debugger/tracer/

@@ +442,5 @@
> +
> +  if (!bestOffsetMapping) {
> +    // XXX: Try not to completely break the experience of using the debugger for
> +    // the user by assuming column 0. Simultaneously, report the error so that
> +    // there is a paper trail if the assumption is bad and the debugging

Nit: s/debugging/tracing/

@@ +444,5 @@
> +    // XXX: Try not to completely break the experience of using the debugger for
> +    // the user by assuming column 0. Simultaneously, report the error so that
> +    // there is a paper trail if the assumption is bad and the debugging
> +    // experience becomes wonky.
> +    reportError(new Error("Could not find a column for offset " + aOffset

Did you copy reportError into this file? I was going to day we should move it into the shared utils, but it looks like we now have a shared error reporter: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/DevToolsUtils.js#23

We should use that here.

@@ +453,5 @@
> +  return bestOffsetMapping.columnNumber;
> +}
> +
> +function timeSinceTraceStarted() {
> +  return +new Date - this._startTime;

What is `this`? I know it is trace actor (right?) but it isn't obvious. We should document what the `this` is in trace type functions (either at the register method or somewhere else) or better yet figure out a way to avoid the need for `this` altogether and pass everything needed as normal parameters.

This function could probably benefit from a documentation comment too.

@@ +456,5 @@
> +function timeSinceTraceStarted() {
> +  return +new Date - this._startTime;
> +}
> +
> +function serializeCompletionValue(aType, aValue) {

Nit: documentation comment.

@@ +506,5 @@
> +    createObjectDescriptor(aValue, aPool, aObjectToId);
> +    return { type: "object", objectId: aObjectToId.get(aValue) };
> +  }
> +
> +  dump("Failed to provide a grip for: " + aValue + "\n");

Nit: Should use that shared error reporting function I linked above.

@@ +543,5 @@
> +  if (aObject.class === "Function") {
> +    if (aObject.name) {
> +      desc.name = aObject.name;
> +    } else if (aObject.displayName) {
> +      desc.displayName = aObject.displayName;

Nit: I know this is copied from the script actors, but I think we really want to prefer (or just use) displayName. The information it conveys is a superset of the info conveyed by name. Maybe Panos can remind me why we don't prefer displayName.
Attachment #778730 - Flags: feedback+
> Nit: I know this is copied from the script actors, but I think we really want to
> prefer (or just use) displayName. The information it conveys is a superset of the
> info conveyed by name. Maybe Panos can remind me why we don't prefer displayName.

And I just realized they end up on different properties of the descriptor. In that case, why is this an "else if" when we could add both in different properties without overwriting the other?
(In reply to Nick Fitzgerald [:fitzgen] from comment #29)
> Comment on attachment 778730 [details] [diff] [review]
> Create trace actor and stub trace client
> 
> Review of attachment 778730 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice job!
> 
> ::: toolkit/devtools/client/dbg-client.jsm
> @@ +1658,5 @@
> > +  }, {
> > +    before: function(aPacket) {
> > +      if (!aPacket.name) {
> > +        aPacket.name = undefined;
> > +      }
> 
> Do we really need to do this? Seems like overkill to me, I think we could
> just skip using the `before` hook here.

The point is to avoid stringifying and sending the name if it's just going to be ignored by the server. It's not really needed though; I'd be fine with removing it.

> @@ +284,5 @@
> > + * ordering. In addition to the push and pop stack operations, supports a
> > + * "remove" operation, which removes the object with a given name from any
> > + * location in the stack.
> > + */
> > +function NamedObjectStack()
> 
> Nit: isn't this more of a `SetStack` since each item is supposed to be
> unique? Personally, I'd like that name better but I can live with
> `NamedObjectStack` if other people disagree with me.

I agree that `NamedObjectStack` is a pretty bad name. It's more of a `MapStack` than a `SetStack` though--elements are removed by key rather than by value.
Addresses fitzgen's feedback comments.
Attachment #778730 - Attachment is obsolete: true
Attachment #778730 - Flags: review?(past)
Attachment #779437 - Flags: review?(past)
Minor fixes; forgot to remove a few things.
Attachment #779437 - Attachment is obsolete: true
Attachment #779437 - Flags: review?(past)
Attachment #779445 - Flags: review?(past)
Comment on attachment 779445 [details] [diff] [review]
Create trace actor and stub trace client

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

::: toolkit/devtools/server/actors/tracer.js
@@ +477,5 @@
> +    // assumption is bad and the tracing experience becomes wonky.
> +    if (Cu.reportError) {
> +      Cu.reportError("TraceActor",
> +                     new Error("Could not find a column for offset " + aOffset +
> +                               " in the script " + aScript));

Sorry, I should have been more clear. You should use the `reportException` function[0] right here directly, not copy its implementation. The thing it also does is dump the error to stdout which is really nice when debugging so that you don't need to go into the error console. We shouldn't repeat this logic all over the place, we should just reuse the existing `reportException` function.

[0]: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/DevToolsUtils.js#23
Attachment #779445 - Flags: feedback+
Use reportException instead of Components.utils.reportError.
Attachment #779445 - Attachment is obsolete: true
Attachment #779445 - Flags: review?(past)
Attachment #779533 - Flags: review?(past)
Remove test call to reportException.
Attachment #779533 - Attachment is obsolete: true
Attachment #779533 - Flags: review?(past)
Attachment #779535 - Flags: review?(past)
(In reply to Nick Fitzgerald [:fitzgen] from comment #30)
> > Nit: I know this is copied from the script actors, but I think we really want to
> > prefer (or just use) displayName. The information it conveys is a superset of the
> > info conveyed by name. Maybe Panos can remind me why we don't prefer displayName.
> 
> And I just realized they end up on different properties of the descriptor.
> In that case, why is this an "else if" when we could add both in different
> properties without overwriting the other?

We don't really need the inferred name when the function is already named, but I had assumed that displayName would be empty when name was present, which appears not to be the case. We could just always use displayName.
(In reply to Panos Astithas [:past] from comment #37)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #30)
> > > Nit: I know this is copied from the script actors, but I think we really want to
> > > prefer (or just use) displayName. The information it conveys is a superset of the
> > > info conveyed by name. Maybe Panos can remind me why we don't prefer displayName.
> > 
> > And I just realized they end up on different properties of the descriptor.
> > In that case, why is this an "else if" when we could add both in different
> > properties without overwriting the other?
> 
> We don't really need the inferred name when the function is already named,
> but I had assumed that displayName would be empty when name was present,
> which appears not to be the case. We could just always use displayName.

The displayName property of Debugger.Object is not the same as if you set the displayName property of a function. I got caught on this too.

+1 for always using displayName (here and in the debugger). Looking at the documentation[0], it seems like it is always more helpful or the same.

Just filed bug 897050.
Comment on attachment 779535 [details] [diff] [review]
Create trace actor and stub trace client

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

This is wonderfully concise and feature-rich at the same time, kudos!

The only thing that kind of saddened me was the preference of copying code over reusing it. I understand that it may have been harder due to some required refactoring, but I have no doubts that we will pay this cost down the road in bugs fixed in one place and not in the other.

I have a number of comments below and I'd also like to see a test that makes sure serialization of a reasonably complex object works as expected, say something like:
{
  a: 1,
  b: {
    c: "c"
  },
  __proto__: {
    d: "foo",
    e: 42,
    __proto__: {
      f: 1
    }
  }
}
(I assume the inner prototype wouldn't be serialized, but still)

What would have been really nice is to make sure event objects are serialized as expected (with safeGetterValues, etc.), as they will be rather common in real world scenarios, but you'd either need a chrome mochitest, or a browser mochitest once the front-end is ready. If you opt for the latter, I guess it's followup material.

::: toolkit/devtools/client/dbg-client.jsm
@@ +501,5 @@
> +   * @param function aOnResponse
> +   *        Called with the response packet and a TraceClient
> +   *        (which will be undefined on error).
> +   */
> +  attachTracer: function DC_attachTracer(aTraceActor, aOnResponse) {

Nit: we don't need to use extra names for functions any more, the engine can infer it on its own.

::: toolkit/devtools/server/actors/tracer.js
@@ +9,5 @@
> +const { reportException } =
> +  Cu.import("resource://gre/modules/devtools/DevToolsUtils.jsm", {}).DevToolsUtils;
> +
> +Cu.import("resource://gre/modules/jsdebugger.jsm");
> +addDebuggerToGlobal(this);

You shouldn't need any of this, we already add the debugger in main.js, so by the time the tracer actor is ready to accept messages the Debugger API is there.

@@ +27,5 @@
> +    this._requestsForTraceType[type] = 0;
> +  }
> +  this._sequence = 0;
> +
> +  this.global = aBrowserTabActor.contentWindow.wrappedJSObject;

Did you check whether this works with Android and B2G? I think it should, but sadly debugger xpcshell tests seem disabled in those two cases at the moment. Fine for a followup, too.

@@ +35,5 @@
> +  actorPrefix: "trace",
> +
> +  get attached() this._attached,
> +  get idle()     this._attached && this._activeTraces.size === 0,
> +  get tracing()  this._attached && this._activeTraces.size > 0,

Could you avoid using this SpiderMonkey-specific way to define getters? Brandon filed bug 887895 to get rid of them and it would be a good idea to stop adding more. Unless of course you don't mind having him hunt you down with a flail!

@@ +162,5 @@
> +
> +    this.dbg = null;
> +
> +    this._attached = false;
> +    this.conn.send({ from: this.actorID, type: "detach" });

"detach" -> "detached"

@@ +174,5 @@
> +   */
> +  onStartTrace: function(aRequest) {
> +    for (let traceType of aRequest.trace) {
> +      if (TraceTypes.types.indexOf(traceType) < 0) {
> +        return { error: "noSuchTraceType" };

There is a standard error type, badParameterType, that we should be using here:
https://wiki.mozilla.org/Remote_Debugging_Protocol#Error_Packets

You could then add a message property with more information about the parameter problem.

@@ +211,5 @@
> +    let stoppedTraceTypes, name;
> +    if (aRequest && aRequest.name) {
> +      name = aRequest.name;
> +      if (!this._activeTraces.has(name)) {
> +        return { error: "noSuchTrace" };

Consider adding "message" properties with elaborate error descriptions in all error messages.

@@ +289,5 @@
> +};
> +
> +exports.register = function(handle) {
> +  handle.addTabActor(TraceActor, "traceActor");
> +};

Aren't we going to use this as a global actor for chrome profiling, too? It's fine if this is planned for a followup.

@@ +305,5 @@
> + */
> +function MapStack()
> +{
> +  this._stack = [];
> +  this._map = Object.create(null);

I think a Map() would have been a perfect fit here, but it's up to you.

@@ +512,5 @@
> +}
> +
> +
> +// Serialization helper functions. Largely copied from script.js and modified
> +// for use in serialization rather than object actor requests.

Can't we reuse code by inheriting from a common ancestor or by defining the common functionality in a traits object?

@@ +530,5 @@
> + *
> + * @return ValueGrip
> + *        A primitive value or a grip object (for null/undefined/objects).
> + */
> +function createValueGrip(aValue, aPool, aObjectToId) {

This version of createValueGrip doesn't know about LongStrings, which will result in needlessly large packets in some cases. Since these value grips only appear in unsolicited packets, there is no room for interaction with long string actors, so I think it would probably make sense to just limit the value to the same length we use for the initial value in LongString actor grips (DebuggerServer.LONG_STRING_INITIAL_LENGTH) plus an ellipsis.

::: toolkit/devtools/server/tests/unit/head_dbg.js
@@ +164,4 @@
>   */
>  function initTestDebuggerServer()
>  {
> +  DebuggerServer.addActors("resource://gre/modules/devtools/server/actors/root.js");

Why is this necessary? The root actor is loaded by init() further down this method. Same question for initTestTraceServer().
Attachment #779535 - Flags: review?(past)
(In reply to Panos Astithas [:past] from comment #39)

> The only thing that kind of saddened me was the preference of copying code
> over reusing it. I understand that it may have been harder due to some
> required refactoring, but I have no doubts that we will pay this cost down
> the road in bugs fixed in one place and not in the other.

I also wasn't too happy about copying all that code, but Nick and I figured that the refactoring would be part of a followup.

> I have a number of comments below and I'd also like to see a test that makes
> sure serialization of a reasonably complex object works as expected, say
> something like:
> {
>   a: 1,
>   b: {
>     c: "c"
>   },
>   __proto__: {
>     d: "foo",
>     e: 42,
>     __proto__: {
>       f: 1
>     }
>   }
> }
> (I assume the inner prototype wouldn't be serialized, but still)

The inner prototype actually is serialized--here's the packet from serializing that object: http://pastebin.mozilla.org/2695634

> Nit: we don't need to use extra names for functions any more, the engine can
> infer it on its own.

I included function names for consistency in files that (mostly) used function names. Should I omit them rather than staying consistent?

> ::: toolkit/devtools/server/actors/tracer.js
> @@ +9,5 @@
> > +const { reportException } =
> > +  Cu.import("resource://gre/modules/devtools/DevToolsUtils.jsm", {}).DevToolsUtils;
> > +
> > +Cu.import("resource://gre/modules/jsdebugger.jsm");
> > +addDebuggerToGlobal(this);
> 
> You shouldn't need any of this, we already add the debugger in main.js, so
> by the time the tracer actor is ready to accept messages the Debugger API is
> there.

The tracer module is added using devtools.require rather than using loadSubscript (which most of the other actors use), so the tracer module is in a different scope--the Debugger object isn't defined without this import. At least, I think that's what's going on--I'm still a little fuzzy on the loader details.

> @@ +27,5 @@
> > +    this._requestsForTraceType[type] = 0;
> > +  }
> > +  this._sequence = 0;
> > +
> > +  this.global = aBrowserTabActor.contentWindow.wrappedJSObject;
> 
> Did you check whether this works with Android and B2G? I think it should,
> but sadly debugger xpcshell tests seem disabled in those two cases at the
> moment. Fine for a followup, too.

I haven't--it may have to be a followup.

> Could you avoid using this SpiderMonkey-specific way to define getters?
> Brandon filed bug 887895 to get rid of them and it would be a good idea to
> stop adding more. Unless of course you don't mind having him hunt you down
> with a flail!

I do mind that :(

> @@ +289,5 @@
> > +};
> > +
> > +exports.register = function(handle) {
> > +  handle.addTabActor(TraceActor, "traceActor");
> > +};
> 
> Aren't we going to use this as a global actor for chrome profiling, too?
> It's fine if this is planned for a followup.

I think this is also planned for a followup.

> @@ +305,5 @@
> > + */
> > +function MapStack()
> > +{
> > +  this._stack = [];
> > +  this._map = Object.create(null);
> 
> I think a Map() would have been a perfect fit here, but it's up to you.

I actually started with a Map, but decided against it after realizing the "same-value" algorithms used in lastIndexOf and Maps have different behavior for -0 and NaN. I figured I could either warn in comments "don't use -0 or NaN as keys", reimplement lastIndexOf with isNaN and some hacky code to detect -0, or restrict keys to strings by using an object. The tracer only uses string keys anyway, so I went with the latter.

> @@ +512,5 @@
> > +}
> > +
> > +
> > +// Serialization helper functions. Largely copied from script.js and modified
> > +// for use in serialization rather than object actor requests.
> 
> Can't we reuse code by inheriting from a common ancestor or by defining the
> common functionality in a traits object?

I would go with the latter, but again, figured this refactoring would be part of a followup.

> This version of createValueGrip doesn't know about LongStrings, which will
> result in needlessly large packets in some cases. Since these value grips
> only appear in unsolicited packets, there is no room for interaction with
> long string actors, so I think it would probably make sense to just limit
> the value to the same length we use for the initial value in LongString
> actor grips (DebuggerServer.LONG_STRING_INITIAL_LENGTH) plus an ellipsis.

That makes sense. I'm concerned about the size of these packets even without LongStrings, though--`return {};` sends a 24,000 character packet (because Object.prototype and everything it references is serialized): http://pastebin.mozilla.org/2696180

> ::: toolkit/devtools/server/tests/unit/head_dbg.js
> @@ +164,4 @@
> >   */
> >  function initTestDebuggerServer()
> >  {
> > +  DebuggerServer.addActors("resource://gre/modules/devtools/server/actors/root.js");
> 
> Why is this necessary? The root actor is loaded by init() further down this
> method. Same question for initTestTraceServer().

Because CommonCreateExtraActors and CommonAppendExtraActors need to be defined before using a TestTabActor, and they're defined in root.js. We could remove this line if they were defined some other way (i.e. in main.js).
(In reply to Jake Bailey from comment #40)
> I also wasn't too happy about copying all that code, but Nick and I figured
> that the refactoring would be part of a followup.

OK, that's acceptable if it's in our near-term plans.

> > (I assume the inner prototype wouldn't be serialized, but still)
> 
> The inner prototype actually is serialized--here's the packet from
> serializing that object: http://pastebin.mozilla.org/2695634

Oh, I see. I didn't realize that we were serializing the whole graph of reachable objects. That could be expensive.

> > Nit: we don't need to use extra names for functions any more, the engine can
> > infer it on its own.
> 
> I included function names for consistency in files that (mostly) used
> function names. Should I omit them rather than staying consistent?

Well, personally, I prefer not to introduce new code with deprecated behavior, but I can see how others might disagree. I'm not hung up on such details, so feel free to do as you please.

> The tracer module is added using devtools.require rather than using
> loadSubscript (which most of the other actors use), so the tracer module is
> in a different scope--the Debugger object isn't defined without this import.
> At least, I think that's what's going on--I'm still a little fuzzy on the
> loader details.

Ah, I see, you are right.

> That makes sense. I'm concerned about the size of these packets even without
> LongStrings, though--`return {};` sends a 24,000 character packet (because
> Object.prototype and everything it references is serialized):
> http://pastebin.mozilla.org/2696180

That's concerning indeed. I had assumed that the event packets would only return sufficient information for the default (or a minimal) presentation of arguments and return values (say, own properties only), with the client asking for more, if the user actually inspected them. Of course that would require object actors that respond to requests, and all the other machinery that is already implemented in the script actors.

On the other hand, collecting all this information in every step of the program could create prohibitively large amounts of data in the backend for mobile devices. Perhaps we need to decide what is the exact feature set we want to provide. In my mind having just a basic inspection capability for objects in a profiler is pretty OK. Collecting everything sounds useful only if we wanted to make a back-in-time sort of debugging tool.

> > Why is this necessary? The root actor is loaded by init() further down this
> > method. Same question for initTestTraceServer().
> 
> Because CommonCreateExtraActors and CommonAppendExtraActors need to be
> defined before using a TestTabActor, and they're defined in root.js. We
> could remove this line if they were defined some other way (i.e. in main.js).

Ah, good point.
Regarding the refactoring comments, I had recommended avoiding large refactorings when Nick first mentioned this as a concern. Would like to keep the impact of this on other code minimal for now. We can consolidate later.

Re: function names, I'm ok with leaving the extra signatures.

My main concerns are Global/Chrome tracing I expect to be tricky. I think it may have to be an out-of-process thing similar to our Browser Debugger.

The large packet sizes have me a bit worried too. Wondering if it's possible to pack them somehow. I fear for mobile environments.
Comment on attachment 779535 [details] [diff] [review]
Create trace actor and stub trace client

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

This is impressive stuff.

Aside from previous concerns about data explosion and chrome tracing, I think this is worth experimenting with.

::: toolkit/devtools/server/actors/tracer.js
@@ +300,5 @@
> +/**
> + * MapStack is a collection of key/value pairs with stack ordering. In
> + * addition to the push and pop stack operations, supports a "delete"
> + * operation, which removes the value associated with a given key from
> + * any location in the stack.

keys are strings? values are objects?

Mostly sugary-coating around a standard JS object, right? And maintains a separate _stack Array to keep track of ordering.

Not sure if it's worth adding the additional comments to this to prevent future generations from having to figure it out or not. Couldn't hurt. :)

@@ +473,5 @@
> +    }
> +  }
> +
> +  if (!bestOffsetMapping) {
> +    // XXX: Try not to completely break the experience of using the

is this associated with the previous TODO? We try to avoid leaving XXX comments in patches to commit unless it's going to be a very close follow-up.

@@ +767,5 @@
> +    try {
> +      desc = aObject.getOwnPropertyDescriptor(name);
> +    } catch (e) {
> +      // Calling getOwnPropertyDescriptor on wrapped native prototypes is not
> +      // allowed (bug 560072).

absorbing exceptions can be bad when trying to debug something. Tempted to reportError this, but also don't want to pollute the console. Probably OK.
Attachment #779535 - Flags: feedback+
(In reply to Rob Campbell [:rc] (:robcee) from comment #43)
> 
> @@ +473,5 @@
> > +    }
> > +  }
> > +
> > +  if (!bestOffsetMapping) {
> > +    // XXX: Try not to completely break the experience of using the
> 
> is this associated with the previous TODO? We try to avoid leaving XXX
> comments in patches to commit unless it's going to be a very close follow-up.

This is copied from on of my recent changes to the debugger.

I've been using XXX comments to designate when something is more tricky/hacky/unexpected than you would think and really requiers your attention to know why we do it that way because it is probably unintuitive. (Not the same set of things that need to have a very close follow up, but perhaps there is an intersection.)

If I've been using it wrong, let me know :)
Addresses Panos' and Rob's comments.
Attachment #779535 - Attachment is obsolete: true
Attachment #781825 - Flags: review?(past)
Comment on attachment 781825 [details] [diff] [review]
Create trace actor and stub trace client

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

This looks fine to me for a first iteration. The only thing I would consider changing, is to limit the amount of information collected for each object, as mentioned in comment 41 (only send the object's own properties instead of the whole prototype chain). This would be better for mobile devices, but it would limit the inspection abilities of the tool.

Rob has thought more about the expected use cases of this tool, so I'll leave it up to him.
Attachment #781825 - Flags: review?(rcampbell)
Attachment #781825 - Flags: review?(past)
Attachment #781825 - Flags: review+
Comment on attachment 781825 [details] [diff] [review]
Create trace actor and stub trace client

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

awesome.

I'd agree with Panos about limiting the initial scope of the object's prototype chain to just own properties. I am worried about datasplosions.

::: toolkit/devtools/server/actors/tracer.js
@@ +784,5 @@
> + *
> + * @return Set
> + *         A Set of names of safe getters.
> + */
> +function findSafeGetters(aObject) {

this is nice. Looks like it could be in a central place, but can do that later if and when we need it.
Attachment #781825 - Flags: review?(rcampbell) → review+
https://hg.mozilla.org/mozilla-central/rev/d8715e2c8b55
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 25
Comment on attachment 778719 [details]
Tracing profiler protocol

beautiful docs, Jake.

copied and converted here for posterity:
https://wiki.mozilla.org/Trace_Actor
Attachment #778719 - Flags: feedback?(dcamp) → feedback+
(In reply to Rob Campbell [:rc] (:robcee) from comment #50)
> Comment on attachment 778719 [details]
> Tracing profiler protocol
> 
> beautiful docs, Jake.
> 
> copied and converted here for posterity:
> https://wiki.mozilla.org/Trace_Actor

Probably belongs in https://github.com/jimblandy/DebuggerDocs with the rest of the RDP docs, no?
Flags: needinfo?(jimb)
By all means, please extend that.

I'd actually like to split up both the Debugger and protocol docs into more pages, for easier bookmarking, browsing and linking.
Flags: needinfo?(jimb)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: