Closed Bug 1449162 Opened 7 years ago Closed 7 years ago

Refactor the NetworkEventActor to use protocol.js

Categories

(DevTools :: Netmonitor, enhancement, P1)

Product:
DevTools
Component:
Netmonitor
Type:
enhancement

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Depends on 1 open bug)

Details

(Whiteboard: dt-fission)

Attachments

(3 files, 2 obsolete files)

Bug 1449162 - Attach longstrings to NetworkEventActor directly instead of WebConsoleActor.
59 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1449162 - Refactor the NetworkEventActor to use protocol.js.
59 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1449162 - Type everything to json and manually manage and marshall.
59 bytes, text/x-review-board-request
jryans
: review+
Details
WebConsoleActor spawns NetworkEventActor instances for the netmonitor. I would like to start converting this one to protocol.js as it may help simplify the code involving `setupInParent`. https://searchfox.org/mozilla-central/rev/003262ae12ce937950ffb8d3b0fa520d1cc38bff/devtools/server/actors/webconsole.js#1968
Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED
Priority: -- → P1
Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=294dd45c6ae17deb933ec66444f90fc47663d50d At the end, it is mostly the first changeset, where I move NetworkEventActor into its own module, that is going to help bug 1444132. But it is great to see an old actor become a protocol.js one!
Attachment #8962757 - Flags: review?(jryans)
Comment on attachment 8962757 [details] Bug 1449162 - Attach longstrings to NetworkEventActor directly instead of WebConsoleActor. https://reviewboard.mozilla.org/r/231602/#review237746 Thanks for working on this! :) I am assuming this commit only moves the actor and no other changes are made here.
Attachment #8962757 - Flags: review?(jryans) → review+
Comment on attachment 8962758 [details] Bug 1449162 - Refactor the NetworkEventActor to use protocol.js. https://reviewboard.mozilla.org/r/231604/#review237756 Thanks for working on this! :) Overall, it looks good to me, but there a few issues to look over before landing. I do wonder how much work it is to convert to p.js fronts as well for these older actors... ::: devtools/server/actors/network-event.js:45 (Diff revision 2) > > - this._timings = {}; > + this._timings = {}; > - this._stackTrace = {}; > + this._stackTrace = {}; > > - // Keep track of LongStringActors owned by this NetworkEventActor. > + // Keep track of LongStringActors owned by this NetworkEventActor. > - this._longStringActors = new Set(); > + this._longStringActors = new Set(); `LongStringActor` is also a protocol.js actor, so it would be great to use the natural parent / child actor lifecycle that p.js provides, instead of this custom tracking. Can you add a separate commit to use that? Let me know if it feels out of scope. ::: devtools/server/actors/network-event.js:75 (Diff revision 2) > > /** > * Releases this actor from the pool. > */ > - release: function() { > + destroy(conn) { > + protocol.Actor.prototype.initialize.call(this, conn); I would think this should be `destroy` instead...? Can this be moved to the end of the function as well? I try to call parent class constructors at the beginning of the self constructor and parent class destroy at the end of the self destroy so there's regular ordering of common code vs. more specific. ::: devtools/server/actors/network-event.js:96 (Diff revision 2) > if (this.channel) { > this.parent._netEvents.delete(this.channel); > } > - this.parent.releaseActor(this); > + > + // Nullify parent before calling releaseActor as it will recall this method > + let parent = this.parent; Would these 3 lines be more naturally part of `release` instead? ::: devtools/server/actors/network-event.js:172 (Diff revision 2) > */ > - onGetRequestPostData: function() { > + getRequestPostData() { > return { > - from: this.actorID, > postData: this._request.postData, > postDataDiscarded: this._discardRequestBody, Should this get the same boolean treatment as `contentDiscarded`? ::: devtools/shared/specs/network-event.js:16 (Diff revision 2) > + generateActorSpec, > + types > +} = require("devtools/shared/protocol"); > + > +types.addDictType("netevent:headers", { > + headers: "json", // This is a dictionnary of string|longstring Nit: dictionary ::: devtools/shared/specs/network-event.js:22 (Diff revision 2) > + headersSize: "number", > + rawHeaders: "nullable:json", // use json as it is a string or longstring > +}); > + > +types.addDictType("netevent:cookies", { > + cookies: "json", // This is a dictionnary of string|longstring Nit: dictionary ::: devtools/shared/specs/network-event.js:77 (Diff revision 2) > + validity: "json", > + fingerprint: "json", > +}); > + > +const networkEventSpec = generateActorSpec({ > + typeName: "netEvent", Should this new actor type be added to the spec index at `specs/index.js`? Looking at that module, it's not very obvious what "types" are meant to be listed there, but it seems like only _actor_ types. I filed bug 1449773 to clarify. ::: devtools/shared/specs/network-event.js:85 (Diff revision 2) > + "network-event-update": { > + type: "networkEventUpdate", > + updateType: Arg(0, "string"), > + > + // request/response headers > + headers: Option(1, "number"), Hmm, so all of these are set to argument index 1, which seems unexpected... Since some fields are sent together, how does it work? Oh, I guess maybe it wouldn't with a p.js front, but for now we're still using old clients. Should we also log bugs to convert old clients to p.js fronts? It seems like we should aim for p.js everywhere if possible...
Attachment #8962758 - Flags: review?(jryans) → review+
Comment on attachment 8963160 [details] Bug 1449162 - Throw an explicit error message when a method specified in a spec is missing in the actor. https://reviewboard.mozilla.org/r/232004/#review237760 Thanks, seems like a helpful improvement! ::: devtools/shared/protocol.js:1107 (Diff revision 1) > console.error("Error reading request: " + packet.type); > throw ex; > } > > + if (!this[spec.name]) { > + throw new Error(`Spec for '${actorProto.typeName}' specify a '${spec.name}' ` + Nit: specifies
Attachment #8963160 - Flags: review?(jryans) → review+
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6) > Comment on attachment 8962757 [details] > Bug 1449162 - Move NetworkEventActor to its own module. > > https://reviewboard.mozilla.org/r/231602/#review237746 > > Thanks for working on this! :) I am assuming this commit only moves the > actor and no other changes are made here. Yes. And it is a valid revision, where netmon still work.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7) > Comment on attachment 8962758 [details] > Bug 1449162 - Refactor the NetworkEventActor to use protocol.js. > > https://reviewboard.mozilla.org/r/231604/#review237756 > > Thanks for working on this! :) Overall, it looks good to me, but there a few > issues to look over before landing. > > I do wonder how much work it is to convert to p.js fronts as well for these > older actors... No idea personaly, It looks like so far, from bug 1289193 we only converted the actors and never the client to fronts? Are all the "decouple fronts from actors in ..." about that? > > ::: devtools/server/actors/network-event.js:45 > (Diff revision 2) > > > > - this._timings = {}; > > + this._timings = {}; > > - this._stackTrace = {}; > > + this._stackTrace = {}; > > > > - // Keep track of LongStringActors owned by this NetworkEventActor. > > + // Keep track of LongStringActors owned by this NetworkEventActor. > > - this._longStringActors = new Set(); > > + this._longStringActors = new Set(); > > `LongStringActor` is also a protocol.js actor, so it would be great to use > the natural parent / child actor lifecycle that p.js provides, instead of > this custom tracking. > > Can you add a separate commit to use that? Let me know if it feels out of > scope. > > ::: devtools/server/actors/network-event.js:75 > (Diff revision 2) > > > > /** > > * Releases this actor from the pool. > > */ > > - release: function() { > > + destroy(conn) { > > + protocol.Actor.prototype.initialize.call(this, conn); > > I would think this should be `destroy` instead...? Oops, wrong copy paste! It helped undercover that this.parent = webConsoleActor was wrong as it overloads `parent` method on `Pool`. So I renamed it to this.webConsoleActor. > ::: devtools/server/actors/network-event.js:96 > (Diff revision 2) > > if (this.channel) { > > this.parent._netEvents.delete(this.channel); > > } > > - this.parent.releaseActor(this); > > + > > + // Nullify parent before calling releaseActor as it will recall this method > > + let parent = this.parent; > > Would these 3 lines be more naturally part of `release` instead? Not sure. If the connection is closed, this will help cleanup WebConsole._actorPool. (release won't be called if we close the connection abruptly) Also, destroy is called after release and we will miss access to `parent` in destroy. All these releaseActor/destroy methods feel ackward as it is very redundant with protocol.js internal pool and destroy mechanism. We will be able to simplify this significantly once WebConsoleActor switches to protocol.js. > ::: devtools/server/actors/network-event.js:172 > (Diff revision 2) > > */ > > - onGetRequestPostData: function() { > > + getRequestPostData() { > > return { > > - from: this.actorID, > > postData: this._request.postData, > > postDataDiscarded: this._discardRequestBody, > > Should this get the same boolean treatment as `contentDiscarded`? Good catch, instead of coercing type in each method, I ensure always setting a boolean everywhere. By setting a default false value in constructor and coercing everywhere we receive a value from NetworkMonitor. I also applied to same to _discardResponseBody. > ::: devtools/shared/specs/network-event.js:77 > (Diff revision 2) > > + validity: "json", > > + fingerprint: "json", > > +}); > > + > > +const networkEventSpec = generateActorSpec({ > > + typeName: "netEvent", > > Should this new actor type be added to the spec index at `specs/index.js`? > > Looking at that module, it's not very obvious what "types" are meant to be > listed there, but it seems like only _actor_ types. I filed bug 1449773 to > clarify. It doesn't have to. It will have to be in this list whenever another protocol.js actor start using it. I added it to the list as it will be used by WebConsoleActor once it moves to protocol.js. > > ::: devtools/shared/specs/network-event.js:85 > (Diff revision 2) > > + "network-event-update": { > > + type: "networkEventUpdate", > > + updateType: Arg(0, "string"), > > + > > + // request/response headers > > + headers: Option(1, "number"), > > Hmm, so all of these are set to argument index 1, which seems unexpected... > Since some fields are sent together, how does it work? This spec event relates to all the calls to emit in the actor: this.emit("network-event-update", updateType, { ... args ...}); Each update type is going to come with different set of "args" (i.e. Option(1, ...) arguments). `arg: Option(1, "x")` means that 1st argument is an object, with an optional `arg` attribute of type `x`. So that it works, but it doesn't ensure that we correctly pass all the expected attributes for each `updateType`. It may be hard to be more correct without changing RDP messages. I think it would require emitting one event per `updateType` instead of sending them all via a unique `network-event-update` event. > Oh, I guess maybe it wouldn't with a p.js front, but for now we're still > using old clients. Even if we convert client to fronts, it will still work as-is. You will receive events like this: front.on("network-event-update", (updateType, args) { if (updateType == "requestHeaders") { let { headers, headersSize } = args; ... } }); > Should we also log bugs to convert old clients to p.js fronts? It seems like we should aim for p.js everywhere if possible... Ideally yes, don't we already have some for at least one actor?
(In reply to Alexandre Poirot [:ochameau] from comment #13) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7) > > Comment on attachment 8962758 [details] > > Bug 1449162 - Refactor the NetworkEventActor to use protocol.js. > > > > https://reviewboard.mozilla.org/r/231604/#review237756 > > > > Thanks for working on this! :) Overall, it looks good to me, but there a few > > issues to look over before landing. > > > > I do wonder how much work it is to convert to p.js fronts as well for these > > older actors... > > No idea personaly, It looks like so far, from bug 1289193 we only converted > the actors and never the client to fronts? > Are all the "decouple fronts from actors in ..." about that? It appears we didn't have any clear bugs for client -> front conversion. I filed bug 1450150 for it. > > ::: devtools/shared/specs/network-event.js:85 > > (Diff revision 2) > > > + "network-event-update": { > > > + type: "networkEventUpdate", > > > + updateType: Arg(0, "string"), > > > + > > > + // request/response headers > > > + headers: Option(1, "number"), > > > > Hmm, so all of these are set to argument index 1, which seems unexpected... > > Since some fields are sent together, how does it work? > > This spec event relates to all the calls to emit in the actor: > this.emit("network-event-update", updateType, { ... args ...}); > Each update type is going to come with different set of "args" (i.e. > Option(1, ...) arguments). > `arg: Option(1, "x")` means that 1st argument is an object, with an optional > `arg` attribute of type `x`. > So that it works, but it doesn't ensure that we correctly pass all the > expected attributes for each `updateType`. > It may be hard to be more correct without changing RDP messages. I think it > would require emitting one event per `updateType` instead of sending them > all via a unique `network-event-update` event. Sorry about this, I think I was confused about how `Option` worked. x_x This part seems fine as-is.
Comment on attachment 8962758 [details] Bug 1449162 - Refactor the NetworkEventActor to use protocol.js. https://reviewboard.mozilla.org/r/231604/#review238140 ::: devtools/server/actors/network-event.js:86 (Diff revisions 2 - 3) > - this.parent.releaseActor(actor); > + this.webConsoleActor.releaseActor(actor); > } > } > this._longStringActors = new Set(); > > - if (!this.parent) { > + if (!this.webConsoleActor) { It's a bit surprising to use `webConsoleActor` immediately _before_ this null check. What's going to make it become null here, when you used it several lines ago? If this is correct, please add a comment explaining why.
Comment on attachment 8963527 [details] Bug 1449162 - Attach longstrings to NetworkEventActor directly instead of WebConsoleActor. https://reviewboard.mozilla.org/r/232468/#review238142 This is probably the right idea, but I think I am not following a key step here... ::: devtools/server/actors/network-event.js:443 (Diff revision 1) > + */ > + _createStringGrip: function(string) { > + if (string && stringIsLong(string)) { > + let actor = new LongStringActor(string); > + this.manage(actor); > + return actor.grip(); Hmm, wait does this actually have a `grip` method? I feel like I must be missing something...
Attachment #8963527 - Flags: review?(jryans) → review-
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #15) > ::: devtools/server/actors/network-event.js:86 > (Diff revisions 2 - 3) > > - this.parent.releaseActor(actor); > > + this.webConsoleActor.releaseActor(actor); > > } > > } > > this._longStringActors = new Set(); > > > > - if (!this.parent) { > > + if (!this.webConsoleActor) { > > It's a bit surprising to use `webConsoleActor` immediately _before_ this > null check. What's going to make it become null here, when you used it > several lines ago? If this is correct, please add a comment explaining why. I don't know. The original code was like this. In the final code, after all the patches are applied, the `if (!this.webConsoleActor)` check is done first. And we set `webConsoleActor` to null just before calling webConsoleActor.releaseActor, which is the call that is going to re-call `destroy` method. (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #16) > Comment on attachment 8963527 [details] > ::: devtools/server/actors/network-event.js:443 > (Diff revision 1) > > + */ > > + _createStringGrip: function(string) { > > + if (string && stringIsLong(string)) { > > + let actor = new LongStringActor(string); > > + this.manage(actor); > > + return actor.grip(); > > Hmm, wait does this actually have a `grip` method? > > I feel like I must be missing something... Ahah. I realized during bug 's review that we are having two LongStringActor implementation. Here, I'm still using the old fashion one, implemented here: https://searchfox.org/mozilla-central/source/devtools/server/actors/object.js#2278 This one implements grip. I should probably try to use the new one, using protocol.js: https://searchfox.org/mozilla-central/source/devtools/server/actors/string.js#12
(In reply to Alexandre Poirot [:ochameau] from comment #17) > Ahah. I realized during bug 's review that we are having two LongStringActor => bug 1449188's review
(In reply to Alexandre Poirot [:ochameau] from comment #17) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #16) > > Comment on attachment 8963527 [details] > > ::: devtools/server/actors/network-event.js:443 > > (Diff revision 1) > > > + */ > > > + _createStringGrip: function(string) { > > > + if (string && stringIsLong(string)) { > > > + let actor = new LongStringActor(string); > > > + this.manage(actor); > > > + return actor.grip(); > > > > Hmm, wait does this actually have a `grip` method? > > > > I feel like I must be missing something... > > Ahah. I realized during bug 's review that we are having two LongStringActor > implementation. > Here, I'm still using the old fashion one, implemented here: > > https://searchfox.org/mozilla-central/source/devtools/server/actors/object. > js#2278 > This one implements grip. > > I should probably try to use the new one, using protocol.js: > > https://searchfox.org/mozilla-central/source/devtools/server/actors/string. > js#12 Haha, that's quite confusing! :X Looks like webconsole is the last to use the old-style string actor for this, so hopefully we can eventually remove it when console actor has been converted.
Comment on attachment 8963527 [details] Bug 1449162 - Attach longstrings to NetworkEventActor directly instead of WebConsoleActor. https://reviewboard.mozilla.org/r/232468/#review238956 ::: devtools/server/actors/network-event.js:443 (Diff revision 2) > + */ > + _createStringGrip: function(string) { > + let actor = new LongStringActor(this.conn, string); > + if (!actor.short) { > + this.manage(actor); > } I'm wondering if this call to manage is important. None of the existing usages of protocol.js version of LongStringActor explicitly manage them. See for example: https://searchfox.org/mozilla-central/source/devtools/server/actors/device.js#56 https://searchfox.org/mozilla-central/source/devtools/server/actors/inspector/node.js#538 It might be redundant with this internal call to manage: https://searchfox.org/mozilla-central/source/devtools/shared/protocol.js#311-316 But I don't think it is the case as we don't return LongStringActor directly, but instead it is on a dictionary attribute. Like here, for `rawHeaders`: https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/devtools/server/actors/webconsole.js#2094
Comment on attachment 8963527 [details] Bug 1449162 - Attach longstrings to NetworkEventActor directly instead of WebConsoleActor. https://reviewboard.mozilla.org/r/232468/#review238956 > I'm wondering if this call to manage is important. > None of the existing usages of protocol.js version of LongStringActor explicitly manage them. > > See for example: > https://searchfox.org/mozilla-central/source/devtools/server/actors/device.js#56 > https://searchfox.org/mozilla-central/source/devtools/server/actors/inspector/node.js#538 > > It might be redundant with this internal call to manage: > https://searchfox.org/mozilla-central/source/devtools/shared/protocol.js#311-316 > But I don't think it is the case as we don't return LongStringActor directly, but instead it is on a dictionary attribute. Like here, for `rawHeaders`: > https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/devtools/server/actors/webconsole.js#2094 For the way you have the patch right now, yes you should call `manage` because (like you said) you aren't returning the actor object, so the p.js internal call never has a chance to run. However, I think we're doing some extra steps here that aren't needed. The `longstring` type has custom marshalling[1] that already handles the case of short vs. long strings. It seems like we should make two changes: 1. Change the spec for long string using methods to actually state `longstring` as the type (or `nullable:longstring` if needed) 2. Always return the long string actor object and let the marshalling figure out what to do This seems to how other users of long string with protocol.js are operating, so it would be good to match if we can. Sorry for missing this before... I forgot about the custom marshaller for long string when reviewing the spec here. :S [1]: https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/devtools/shared/specs/string.js#65
Comment on attachment 8963527 [details] Bug 1449162 - Attach longstrings to NetworkEventActor directly instead of WebConsoleActor. https://reviewboard.mozilla.org/r/232468/#review238998 Using the `longstring` type for real (so that the marshaller is used) seems like the right path here. I'd like to see that patch (or if it can't be used here, I'd like to know that as well) so marking r- for now.
Attachment #8963527 - Flags: review?(jryans) → review-
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #26) > Comment on attachment 8963527 [details] > Bug 1449162 - Attach longstrings to NetworkEventActor directly instead of > WebConsoleActor. > > https://reviewboard.mozilla.org/r/232468/#review238998 > > Using the `longstring` type for real (so that the marshaller is used) seems > like the right path here. I'd like to see that patch (or if it can't be > used here, I'd like to know that as well) so marking r- for now. Oh well, I thought headers and cookies attributes were dictionaries. So I came up with a tweak to protocol.js to support "unified dict type". A type with a dictionary where you don't know what will be the property names, but you have a fixed type for the property values. But afterall, I ended up understanding that these two headers and cookies fields are arrays... So it is much easier to support with correct type. At the end I tweaked the spec definition in the second patch and modified the last to return actors instead of grips/forms. (FYI: I didn't knew about custom marshalling before your previous comment)
I just pushed an extra change to fix try failures, you may want to take an extra look at stylesheets.js tweaks: https://reviewboard.mozilla.org/r/232468/diff/3-4/ I also learned today that style editor is reusing netmonitor clients/fronts...
(In reply to Alexandre Poirot [:ochameau] from comment #30) > (FYI: I didn't knew about custom marshalling before your previous comment) Yeah, I don't think it's used often...? It is briefly discussed in the p.js docs: https://dxr.mozilla.org/mozilla-central/rev/00bdc9451be6557ccce1492b9b966d4435615380/devtools/docs/backend/protocol.js.md#200 At least 4 usages in specs, so still pretty rare: https://searchfox.org/mozilla-central/search?q=addType&case=true&regexp=false&path=specs
(In reply to Alexandre Poirot [:ochameau] from comment #32) > I just pushed an extra change to fix try failures, you may want to take an > extra look at stylesheets.js tweaks: > https://reviewboard.mozilla.org/r/232468/diff/3-4/ > > I also learned today that style editor is reusing netmonitor > clients/fronts... Ah yeah, I guess that's my doing... :S It was a bit of an experiment to see if it's worthwhile. It does seem worthwhile to shared loaded resources in some way. I wouldn't suggest using this same approach I used here for Style Editor in other tools as it's quite a hack (though we could...). Instead, I am hopeful we can build a central "resources" storage and then any tool can check there for loaded files (instead of reaching into the netmonitor like this). Anyway, just some thoughts about the future.
Comment on attachment 8963527 [details] Bug 1449162 - Attach longstrings to NetworkEventActor directly instead of WebConsoleActor. https://reviewboard.mozilla.org/r/232468/#review239340 I think there are a few more tweaks needed here. It's quite close though. Thanks for sticking with it; I know this stuff is a bit obscure... ::: devtools/server/actors/network-event.js:443 (Diff revisions 2 - 4) > * A string is returned if |string| is not a long string. > - * A LongStringActor grip is returned if |string| is a long string. > + * A LongStringActor is returned if |string| is a long string. > */ > - _createStringGrip: function(string) { > + _createLongString: function(string) { > let actor = new LongStringActor(this.conn, string); > if (!actor.short) { Can this if block be removed now? I was hoping the `longstring` marshalling would cause p.js to handle this internally now... ::: devtools/server/actors/stylesheets.js:478 (Diff revision 4) > } > let content = request._response.content; > if (request._discardResponseBody || request._truncated || !content) { > return null; > } > - if (content.text.type != "longString") { > + if (typeof content.text.type == "string") { Hmm... I am not sure I follow this part... Prior to your work in this bug, `content.text` would be the string directly when short or the long string actor (from object.js) form when long.[1] After your patches here, `content.text` is a always a `LongStringActor` object (from string.js) which may or may not be "short" internally. Assuming that's all correct, it seems like we can remove the short vs. long distinction in this function, since we always have an actor object with the full string in `content.text.str`. [1]: https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/devtools/server/actors/webconsole.js#516
Attachment #8963527 - Flags: review?(jryans) → review-
Comment on attachment 8962758 [details] Bug 1449162 - Refactor the NetworkEventActor to use protocol.js. https://reviewboard.mozilla.org/r/231604/#review239362 ::: devtools/shared/specs/network-event.js:15 (Diff revision 5) > + RetVal, > + generateActorSpec, > + types > +} = require("devtools/shared/protocol"); > + > +types.addDictType("netevent#headers", { I am somewhat perplexed by the usage of `a#b` in the type names, since `#` is also used for the "detail" form feature[1]. Is this usage of `#` about the detail feature, or are you trying to give a name to a type that is a field of something else? If it's not about form details, I think a different character would be better to reduce confusion. Maybe `a.b` instead? [1]: https://dxr.mozilla.org/mozilla-central/rev/00bdc9451be6557ccce1492b9b966d4435615380/devtools/docs/backend/protocol.js.md#436
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #35) > Comment on attachment 8963527 [details] > Bug 1449162 - Attach longstrings to NetworkEventActor directly instead of > WebConsoleActor. > > https://reviewboard.mozilla.org/r/232468/#review239340 > > I think there are a few more tweaks needed here. It's quite close though. > Thanks for sticking with it; I know this stuff is a bit obscure... I was expecting some dark corner. It was great to learn so many things! > ::: devtools/server/actors/network-event.js:443 > (Diff revisions 2 - 4) > > * A string is returned if |string| is not a long string. > > - * A LongStringActor grip is returned if |string| is a long string. > > + * A LongStringActor is returned if |string| is a long string. > > */ > > - _createStringGrip: function(string) { > > + _createLongString: function(string) { > > let actor = new LongStringActor(this.conn, string); > > if (!actor.short) { > > Can this if block be removed now? I was hoping the `longstring` marshalling > would cause p.js to handle this internally now... I was overthinking I think. LongStringActor instances related to short strings are going to stay around until the connection is close of NetworkEventActor is released, whereas the instances for really long strings can be released as soon as client release them. But I'm not sure netmonitor client does such thing. I also removed _createLongString helper and instanciate LongStringActor directly everywhere. > ::: devtools/server/actors/stylesheets.js:478 > (Diff revision 4) > > } > > let content = request._response.content; > > if (request._discardResponseBody || request._truncated || !content) { > > return null; > > } > > - if (content.text.type != "longString") { > > + if (typeof content.text.type == "string") { > > Hmm... I am not sure I follow this part... > > Prior to your work in this bug, `content.text` would be the string directly > when short or the long string actor (from object.js) form when long.[1] > > After your patches here, `content.text` is a always a `LongStringActor` > object (from string.js) which may or may not be "short" internally. > > Assuming that's all correct, it seems like we can remove the short vs. long > distinction in this function, since we always have an actor object with the > full string in `content.text.str`. Yes, that's correct. I fixed that.
(In reply to Alexandre Poirot [:ochameau] from comment #40) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #35) > > ::: devtools/server/actors/network-event.js:443 > > (Diff revisions 2 - 4) > > > * A string is returned if |string| is not a long string. > > > - * A LongStringActor grip is returned if |string| is a long string. > > > + * A LongStringActor is returned if |string| is a long string. > > > */ > > > - _createStringGrip: function(string) { > > > + _createLongString: function(string) { > > > let actor = new LongStringActor(this.conn, string); > > > if (!actor.short) { > > > > Can this if block be removed now? I was hoping the `longstring` marshalling > > would cause p.js to handle this internally now... > > I was overthinking I think. > LongStringActor instances related to short strings are going to stay around > until the connection is close of NetworkEventActor is released, whereas the > instances for really long strings can be released as soon as client release > them. But I'm not sure netmonitor client does such thing. > > I also removed _createLongString helper and instanciate LongStringActor > directly everywhere. I don't think I am seeing these changes in the latest diff, is something missing? https://reviewboard.mozilla.org/r/232468/diff/4-5/
Flags: needinfo?(poirot.alex)
Here you go, I forgot to attach the change to remove _createLongString. (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #36) > I am somewhat perplexed by the usage of `a#b` in the type names, since `#` > is also used for the "detail" form feature[1]. I didn't knew this feature, I saw this comment and thought "#" would be a better separator than ":". https://searchfox.org/mozilla-central/source/devtools/shared/protocol.js#43-47 > Is this usage of `#` about the detail feature, or are you trying to give a > name to a type that is a field of something else? > > If it's not about form details, I think a different character would be > better to reduce confusion. Maybe `a.b` instead? I switched to use ".". My intent was to avoid using ":" which is use for other purposes.
Flags: needinfo?(poirot.alex)
Comment on attachment 8963527 [details] Bug 1449162 - Attach longstrings to NetworkEventActor directly instead of WebConsoleActor. https://reviewboard.mozilla.org/r/232468/#review239406 Thanks for working on this! :) Looks good to me; glad we worked out those details!
Attachment #8963527 - Flags: review?(jryans) → review+
I tried various ways to improve protocol.js performance where I saw possibly slow code pattern. # Preventing the duplication of arrays doesn't seem to impact any test: https://hg.mozilla.org/try/rev/8a936276f48eca90b0b06846e7e05d3c0916c9ee https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=5e04a12875369e81741856a124dc856f60147497&newProject=try&newRevision=180761146a4972d0440e47db9c982ca2282fae77&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyConfident=1&framework=1 # Prevent duplicating arrays in identityWrite, doesn't seem to impact any test either: https://hg.mozilla.org/try/rev/dbddbf6644119d3ecfcb95c64d6c589d7dfb5efc https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=5e04a12875369e81741856a124dc856f60147497&newProject=try&newRevision=952f236237210f1abddd68cc7127bf94200a0844&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyConfident=1&framework=1 # Prevent duplicating "template" objects in Request and Response's write methods, this time impacts tests: https://hg.mozilla.org/try/rev/bab6fec74303 https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=5e04a12875369e81741856a124dc856f60147497&newProject=try&newRevision=87983cf1a5948db664c73368ebd13a2a5143b2ba&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyConfident=1&framework=1 It allows to revert back 2.6% of the 8.3% regression of netmonitor refactoring patch. But it also regress various other tests, so I'll look again at this patch to see if it has anything wrong.
(In reply to Alexandre Poirot [:ochameau] from comment #47) > # Prevent duplicating "template" objects in Request and Response's write > methods, this time impacts tests About this, I split the two modifications and it looks like Response.write tweak improves the perf in many cases (and regress a couple which doesn't look too bad) https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=8e604bbe8e62&newProject=try&newRevision=4570da82eae1035ee3fb16b12ae9411b78b20da3&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyConfident=1&framework=1 While Request.write only regress things: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=8e604bbe8e62&newProject=try&newRevision=d43a11a048b57e3b93431e1a0c09ad9a3ce92804&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyConfident=1&framework=1 It is very surprising to see that I no longer get any improvement on har export test. I will try to better understand these write method, they are still unclear to me.
Depends on: 1453385
(In reply to Alexandre Poirot [:ochameau] from comment #45) > Hum. Unfortunately, it looks like it regress performance of HAR export: > https://treeherder.mozilla.org/perf.html#/ > comparesubtest?originalProject=try&originalRevision=ca7d721110ab4341fe4fcb19a > 5eae9fecab9a61a&newProject=try&newRevision=deeefa937fe8d39ea6312bb4b1f88cdd9d > b5d2aa&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignatur > e=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyImportant=1&framework=1 > 16% regression on complicated.netmonitor.exportHar > The rest may be related to this one test being significantly slower. Unfortunately, bug 1453385 doesn't help that much. Here is a try with on top of it and comparing actor refactoring to protocol.js: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=5160e6fc6c7035318e5307e3b4040401b7be7ab9&newProject=try&newRevision=218877fefb2e8aa2e09c50f71bb3a5641d165c43&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyConfident=1&framework=1 We slim down the regression from 16% to 14% only. Here is two profile of DAMP during exporthar test: * one without the patches (bug 1453385 and this one): https://perfht.ml/2IUBACL * another one with netmonitor actor refactoring only: https://perfht.ml/2IRhaKX We can see in the second one 420ms spent in protocol.js and the test go from 5.4s to 7.0s.
(In reply to Alexandre Poirot [:ochameau] from comment #50) > Here is two profile of DAMP during exporthar test: > * one without the patches (bug 1453385 and this one): > https://perfht.ml/2IUBACL > * another one with netmonitor actor refactoring only: > https://perfht.ml/2IRhaKX Do you have a profile _with_ both this bug and bug 1453385 applied? There's a Talos link for that above I guess... Can you jump to a profile from Talos yet?
Flags: needinfo?(poirot.alex)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #51) > (In reply to Alexandre Poirot [:ochameau] from comment #50) > > Here is two profile of DAMP during exporthar test: > > * one without the patches (bug 1453385 and this one): > > https://perfht.ml/2IUBACL > > * another one with netmonitor actor refactoring only: > > https://perfht.ml/2IRhaKX > > Do you have a profile _with_ both this bug and bug 1453385 applied? There's > a Talos link for that above I guess... Can you jump to a profile from Talos > yet? Not yet, and you will need special talos runs for having profile as it slow down everything. I redid profiles as it easily change based on weather, temperature and humidity (but also base m-c changeset): * m-c (9299ms): https://perfht.ml/2qvtCJH transport.js (390ms) * with protocol fix (8952ms): https://perfht.ml/2qurVfw transport.js (370ms) * with protocol fix and networkeventactor (9642ms): https://perfht.ml/2HklCox transport.js (750ms) protocol.js (820ms) protocol.js, write (150ms) * with additional fixes I haven't attached to bugzilla (10112ms): https://perfht.ml/2qvrVfj transport.js (560ms) protocol.js (590ms) protocol.js, write (160ms) First thing is that the improvement isn't obvious with just one run *and* the profiler turned on. I see a couple of stacks related to Promise.jsm from protocol.js, I'm wondering if it could be faster if we use native promises here.
Flags: needinfo?(poirot.alex)
* switch to native Promises (9630ms) [plus additional fixes and all other patches] https://perfht.ml/2HlFTdb protocol.js (320ms) protocol.js, write (60ms) It may have lowered the cost of protocol.js as Promise.jsm could included the cost of some promise resolution handler that weren't related to protocol.js.
(In reply to Alexandre Poirot [:ochameau] from comment #53) > * switch to native Promises (9630ms) [plus additional fixes and all other Actually, it looks like Promise.jsm, in the case of protocol.js, introduces an overhead. https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=b800a3fdec0db81820c3a77836f475de4176ea26&newProject=try&newRevision=f9a1ec00d6df5e2c7074f1df01d980eb2b727bd3&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyConfident=1&framework=1 DAMP reports 8.4% improvement on har export test and between 2% and 5% on a couple of reload tests. But may be I broke something, let's see if try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=940887aa1362afa661f4b9a1d258f82e4934557e
Depends on: 1454373
(In reply to Alexandre Poirot [:ochameau] from comment #54) > (In reply to Alexandre Poirot [:ochameau] from comment #53) > > * switch to native Promises (9630ms) [plus additional fixes and all other Good news, try was almost green, so it looks like Promise.jsm was the second biggest source of slowness. export har test now only regress by 4% with bug 1454373: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=183d6db2b680&newProject=try&newRevision=751b03561072&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyConfident=1&framework=1
Depends on: 1454792
Depends on: 1454899
Attachment #8963527 - Attachment is obsolete: true
Depends on: 1459205
Depends on: 1460228
Depends on: 1460229
I'm starting to get conflicts when rebasing, so I'll try to land whatever is safe landing.
Attachment #8963160 - Attachment is obsolete: true
Also, to confirm it isn't related to the long string actor refactoring, a DAMP run with only the first patch highlights similar regression: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=1983e2e0f693ec67d3fd921f75dfcbd21237fa40&newProject=try&newRevision=ec344f61da8ab846b52cddc5af84c63edf55786a&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyImportant=1&framework=1 Otherwise, I thought that the regression came from the very explicit types, where we have to call all these "write" methods. So I switched to `RetVal('json')` everywhere, so that it would pass through most the data, but surprisingly, it regressed the perf of exportHar: https://hg.mozilla.org/try/rev/1b5bcc9c541609810eb3e3c1bc80a579e15a5b51 https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=ec344f61da8a&newProject=try&newRevision=791137c31fb9a4b7d352edfbc6ef271bc746a77e&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyImportant=1&framework=1 "json" is a primitive type in protocol.js and its read/write methods are identityWrite which just pass the object as-is. It is surprising to not see any improvement as I see a lot of calls to all the write methods in the content process related to the explicit types...
With the latest patch, splitting "network-event-update" into individual events at protocol.js level removes a significant part of the regression. We still emit only one event at RDP level "networkEventUpdate", but having multiple ones at protocol.js allows to not process all "Option" types for each message. We were spending a significant time into Request.write because of that. Unfortunately, regressions are still reported. We go from 7% down to 2% on complicated.requestsFinished and from 10% down to 2% on complicated.reload. complicated.exportHar looks worse for some reason, it goes from 2.5% up to 4%: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=c2a73ed25818ff1d064d117bdcc905d6fff1cf1e&newProject=try&newRevision=a1634d0ecca5a6ce0b53fe9b3a5adacee1c593c0&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyImportant=1&framework=1 Overall the regression looks much more reasonable, but this is still a good case to study protocol.js performance.
I hope to have found the last significant regression. See comment 66 and https://reviewboard.mozilla.org/r/231604/diff/8-9/ for the previous perf fix, where having lots of Option() was a perf bottleneck. Now this patch address the overhead of Response.write when using explicit types (anything else but `json`/primitive types). So I'm switching to `json` in many places, just to avoid procotol.js `write` methods. There is two issues with that: * we ignore protocol.js typing/definition system... * we have to manually manage and marshall actors returned by actor's methods. I just spawn new DAMP runs, but I hope, with that last patch, to present no, or reasonable regression(s).
Waiting for new DAMP runs before review after chatting about this with :ochameau.
DAMP results were very confusing because one run had a lot of noise. I'll look into that as when it happens no more regression are reported! Here is a first run with just the first patch: Refactor the NetworkEventActor to use protocol.js. https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=fa0521aad96c18e2185d27648ecc2073d4b25ad4&newProject=try&newRevision=fdd30d916fb7125a4fbf0f86806375d4affd8380&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyConfident=1&framework=1 complicated.netmonitor.requestsFinished +1.53% (med) complicated.netmonitor.exportHar -1.77% (med) Another with the two first patches: Attach longstrings to NetworkEventActor directly instead of WebConsoleActor. https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=fa0521aad96c&newProject=try&newRevision=8a4b606f43fcc2b11e441683756187889e4c5a5c&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyConfident=1&framework=1 complicated.netmonitor.requestsFinished +1.59% (high) complicated.netmonitor.exportHar +3.96% (high) And a last one with all of them: Type everything to json and manually manage and marshall. https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=fa0521aad96c&newProject=try&newRevision=958153033f9d779833418df8fc028a4d04313742&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyConfident=1&framework=1 complicated.netmonitor.requestsFinished +2.67% (high) complicated.netmonitor.exportHar +1.43% (high) Also, an interdiff between 1st and 2nd patch: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=fdd30d916fb7125a4fbf0f86806375d4affd8380&newProject=try&newRevision=8a4b606f43fcc2b11e441683756187889e4c5a5c&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyConfident=1&framework=1 complicated.netmonitor.exportHar +5.83% (high) And between 2nd and 3rd: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=8a4b606f43fcc2b11e441683756187889e4c5a5c&newProject=try&newRevision=958153033f9d779833418df8fc028a4d04313742&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyImportant=1&framework=1 complicated.netmonitor.exportHar -2.44% (high) So. The last patch mostly accomodate the regression made to exportHAR by the switch to protocol.js's LongStrings. There is still a regression at the end, but this time it is very small on exportHar. I wish I could also reduce the one on requestsFinished, but it would require more invasive modification to protocol.js. Like getting rid of mappings or optimizing the automatic marshalling of actors. I would like to keep looking at protocol.js to eventually revert the 3rd patch.
Comment on attachment 8976119 [details] Bug 1449162 - Type everything to json and manually manage and marshall. https://reviewboard.mozilla.org/r/244302/#review250782 Seems like an important perf boost even though it removes one area that p.js is useful for. ::: devtools/server/actors/network-event.js:278 (Diff revision 1) > this._request.headers = headers; > this._prepareHeaders(headers); > > if (rawHeaders) { > rawHeaders = new LongStringActor(this.conn, rawHeaders); > + this.manage(rawHeaders); Could you add a comment at each of these call sites to mention that this manual marshalling is done for performance for the moment, but we hope to improve p.js so that it's not needed? Maybe you can file a bug (if there's not one already) about the future improvement and mention in these comments.
Attachment #8976119 - Flags: review?(jryans) → review+
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1784812cfb14 Refactor the NetworkEventActor to use protocol.js. r=jryans https://hg.mozilla.org/integration/autoland/rev/56430907e655 Attach longstrings to NetworkEventActor directly instead of WebConsoleActor. r=jryans https://hg.mozilla.org/integration/autoland/rev/6c0ff9599fc9 Type everything to json and manually manage and marshall. r=jryans
https://hg.mozilla.org/mozilla-central/rev/1784812cfb14 https://hg.mozilla.org/mozilla-central/rev/56430907e655 https://hg.mozilla.org/mozilla-central/rev/6c0ff9599fc9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Perf wins! == Change summary for alert #13437 (as of Thu, 24 May 2018 22:17:55 GMT) == Improvements: 6% damp windows10-64 pgo e10s stylo 85.04 -> 80.26 4% damp windows10-64 opt e10s stylo 89.85 -> 86.38 3% damp windows7-32 pgo e10s stylo 86.90 -> 83.91 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=13437
(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #86) > Perf wins! > > == Change summary for alert #13437 (as of Thu, 24 May 2018 22:17:55 GMT) == > > Improvements: > > 6% damp windows10-64 pgo e10s stylo 85.04 -> 80.26 > 4% damp windows10-64 opt e10s stylo 89.85 -> 86.38 > 3% damp windows7-32 pgo e10s stylo 86.90 -> 83.91 > > For up to date results, see: > https://treeherder.mozilla.org/perf.html#/alerts?id=13437 Sorry, I have misplaced this notification.
Depends on: 1465778
Depends on: 1462561
Product: Firefox → DevTools
Whiteboard: dt-fission
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: