1921200 - Revise the implementation of grip's depth

Status: RESOLVED FIXED

(DevTools :: Debugger, task)

Description

[Alexandre Poirot [:ochameau] [PTO back Jan 5th]]

The depth of grips is current managed via hooks provided by the instantiator of Object Actors:
https://searchfox.org/mozilla-central/rev/6b6c3965d0a79880493b8ae44a92389b72d90636/devtools/server/actors/object/utils.js#575,581-583
https://searchfox.org/mozilla-central/rev/6b6c3965d0a79880493b8ae44a92389b72d90636/devtools/server/actors/thread.js#1782-1784
https://searchfox.org/mozilla-central/rev/6b6c3965d0a79880493b8ae44a92389b72d90636/devtools/server/actors/webconsole.js#428-430

But this is brittle as concurrent call may confuse the shared grip depth counter.
This also introduce complexity as we have to pass down these hooks function in previewers and grip generator functions.

It would be clearer and easier if the depth was handed over from function to function within the previewer codebase.

Thanks can also be simplified around createValueGrip. There is too many such functions.

Comment 1

[Alexandre Poirot [:ochameau] [PTO back Jan 5th]]

Attachment: Bug 1921200 - [devtools] Use a single method to create all the Object Actors.

This will help get rid of hooks.

Comment 2

[Alexandre Poirot [:ochameau] [PTO back Jan 5th]]

Attachment: Bug 1921200 - [devtools] Move _propertyDescriptor to an independant method.

This will later help receive something else than an objectActor as argument.
This will also help pass down the right depth via an options object.

Comment 3

[Alexandre Poirot [:ochameau] [PTO back Jan 5th]]

Attachment: Bug 1921200 - [devtools] Expose rawObj and safeRawObj on ObjectActor.

This clarifies the distinction between the two and help use safeRawObj from previewers.

Comment 4

[Alexandre Poirot [:ochameau] [PTO back Jan 5th]]

Attachment: Bug 1921200 - [devtools] Expose className on Object Actor.

This helps use this from previewers.

Comment 5

[Alexandre Poirot [:ochameau] [PTO back Jan 5th]]

Attachment: Bug 1921200 - [devtools] Stop using hooks to pass object grip depth.

Instead, pass a variable down to each function requiring it.
This drastically simplify the story around depth management,
and no longer face race condition if these hooks method were called concurrently.

Comment 6

[Alexandre Poirot [:ochameau] [PTO back Jan 5th]]

Attachment: Bug 1921200 - [devtools] Stop using hooks to call createValueGrip.

For this, we need to be more explicit about the current depth and pass it
from each callsite.

Comment 7

[Alexandre Poirot [:ochameau] [PTO back Jan 5th]]

Attachment: Bug 1921200 - [devtools] Ensure destroying Object Actor on release.

The actors were kept, destroyed, in their pool.
It should help free some more cycles and memory.

Comment 8

[Alexandre Poirot [:ochameau] [PTO back Jan 5th]]

https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=6a9d620589cf5c0487da3675cd3f903696c5d295&originalSignature=4763542&newSignature=4763542&framework=12&application=firefox&originalRevision=30e0c03c9b4c63709ddb47fc53a9968ee9482b2d&page=1&showOnlyConfident=1&pageTitle=whole+patch+queue

Some improvements, 3 to 10% on various types when running console.log-in-loop-content-process-* tests.
But still a 34% regression on node type.
I got a much higher regression due to an issue around depth, I'm wondering if there is still one hidden somewhere.
I'll followup on this.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9acaa9459ff5
[devtools] Use a single method to create all the Object Actors. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/69bf4e17063f
[devtools] Move _propertyDescriptor to an independant method. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/f6b44bfde8b8
[devtools] Expose rawObj and safeRawObj on ObjectActor. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/cf8af58e5624
[devtools] Ensure destroying Object Actor on release. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/876c923f770e
[devtools] Expose className on Object Actor. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/6e2c5c0edc4a
[devtools] Stop using hooks to pass object grip depth. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/9b1af84f3f93
[devtools] Stop using hooks to call createValueGrip. r=devtools-reviewers,nchevobbe

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/9acaa9459ff5
https://hg.mozilla.org/mozilla-central/rev/69bf4e17063f
https://hg.mozilla.org/mozilla-central/rev/f6b44bfde8b8
https://hg.mozilla.org/mozilla-central/rev/cf8af58e5624
https://hg.mozilla.org/mozilla-central/rev/876c923f770e
https://hg.mozilla.org/mozilla-central/rev/6e2c5c0edc4a
https://hg.mozilla.org/mozilla-central/rev/9b1af84f3f93

Comment 11

[Alexandre Poirot [:ochameau] [PTO back Jan 5th]]

Even after rebasing this patch queue on top of bug 1923336, which fixes DOM Element logging performance tracking in DAMP,
I still get a 18% regression on console.log-in-loop-content-process-node.
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=ac3ada97480b4b2fdfa952bf1619c5f1013ac3c9&originalSignature=4763542&newSignature=4763542&framework=12&application=firefox&originalRevision=902a9500a21b3ddd15572e1d7897aa4c1f7293aa&page=1&showOnlyConfident=1

I may have changed the behavior of it, but it is unclear what.