977009 - Use Cu.cloneInto() instead of JSON.parse(JSON.stringify(obj))

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

JSON.parse(JSON.stringify()) is slow. We should rather use Cu.cloneInto() when we want to clone an object. I found several occurrences in devtools code:

browser/devtools/framework/connect/connect.js:87:    let globals = JSON.parse(JSON.stringify(aResponse));
browser/devtools/profiler/cleopatra/js/parserWorker.js:704:  gProfiles[profileID] = JSON.parse(JSON.stringify({
browser/devtools/shared/Parser.jsm:428:        let loc = JSON.parse(JSON.stringify(parentLocation));
browser/devtools/shared/Parser.jsm:436:        let loc = JSON.parse(JSON.stringify(parentLocation));
browser/devtools/shared/Parser.jsm:444:        let loc = JSON.parse(JSON.stringify(parentLocation));
browser/devtools/shared/Parser.jsm:452:        let loc = JSON.parse(JSON.stringify(parentLocation));
browser/devtools/shared/Parser.jsm:462:        let loc = JSON.parse(JSON.stringify(nodeLocation));
browser/devtools/webconsole/hudservice.js:194:          let globals = JSON.parse(JSON.stringify(aResponse));

If you want to clone |obj| into a new object you can do:

let newObj = Cu.cloneInto(obj, {});

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

But wait, there is more:

toolkit/devtools/server/actors/profiler.js:174:    let subj = JSON.parse(JSON.stringify(aSubject, cycleBreaker));
toolkit/devtools/server/actors/profiler.js:175:    let data = JSON.parse(JSON.stringify(aData,    cycleBreaker));
toolkit/devtools/server/actors/root.js:354:    return JSON.parse(JSON.stringify(aRequest));
toolkit/devtools/server/protocol.js:608:    return JSON.parse(JSON.stringify(this.template, function(key, value) {

Comment 2

[Victor Porof [:vporof][:vp]]

Nice! <3

Comment 3

[:Gijs (he/him)]

Attachment: switch to Cu.cloneInto instead of JSON.parse(JSON.stringify(foo)) because it's faster,

I haven't touched all of these cases, because there's some that use a second argument to JSON.stringify to filter output, and cloneInto can't do that. Try push: https://tbpl.mozilla.org/?tree=Try&rev=9fa3dea02817

Comment 4

[Victor Porof [:vporof][:vp]]

Comment on attachment 8428663 [details] [diff] [review]
switch to Cu.cloneInto instead of JSON.parse(JSON.stringify(foo)) because it's faster,

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

Assuming all tests still pass.

Comment 5

[:Gijs (he/him)]

Green apart from an intermittent, which I've filed as bug 1016024

remote:   https://hg.mozilla.org/integration/fx-team/rev/de783743ebcd

Comment 6

[:Gijs (he/him)]

Sigh. Except xpcshell tests (which I didn't think to include in my trypushes) went orange, because Cu is not defined in the server root actor:

remote:   https://hg.mozilla.org/integration/fx-team/rev/fa3cb3c36c30

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Pretty sure this is going to break Eddy's work of making the server run without chrome code so that it can be in a worker.

Comment 8

[Eddy Bruel [:ejpbruel]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #7)
> Pretty sure this is going to break Eddy's work of making the server run
> without chrome code so that it can be in a worker.

Yes, this will break the worker debugger, where Cu is not available. We might be able to come up with a replacement API for Cu.cloneInto in a worker, but we should hold off on this until worker debugging lands.

Comment 9

[:Gijs (he/him)]

(In reply to Eddy Bruel [:ejpbruel] from comment #8)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #7)
> > Pretty sure this is going to break Eddy's work of making the server run
> > without chrome code so that it can be in a worker.
> 
> Yes, this will break the worker debugger, where Cu is not available. We
> might be able to come up with a replacement API for Cu.cloneInto in a
> worker, but we should hold off on this until worker debugging lands.

I'm not clear on what you mean here. Do you mean this patch needs to be backed out?

Comment 10

[Eddy Bruel [:ejpbruel]]

(In reply to :Gijs Kruitbosch from comment #9)
> (In reply to Eddy Bruel [:ejpbruel] from comment #8)
> > (In reply to Nick Fitzgerald [:fitzgen] from comment #7)
> > > Pretty sure this is going to break Eddy's work of making the server run
> > > without chrome code so that it can be in a worker.
> > 
> > Yes, this will break the worker debugger, where Cu is not available. We
> > might be able to come up with a replacement API for Cu.cloneInto in a
> > worker, but we should hold off on this until worker debugging lands.
> 
> I'm not clear on what you mean here. Do you mean this patch needs to be
> backed out?

Actually, we should be able to do is something like:

if (Cu.cloneInto)
  Cu.cloneInto(...)
else
  JSON.parse(JSON.stringify)

to make it work inside a worker, in which case this is fine, and you don't have to do anything.

But please, in the future, notify me of these things *before* you land them!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/de783743ebcd
https://hg.mozilla.org/mozilla-central/rev/fa3cb3c36c30

Comment 12

[:Gijs (he/him)]

Marco, can you add this to this iteration?

Comment 13

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #12)
> Marco, can you add this to this iteration?

Comment 14

[Marco Mucci [:MarcoM]]

Added to Iteration 32.3