1451025 - Convert SymbolIteratorActor to protocol.js

Status: RESOLVED FIXED

(DevTools :: General, enhancement)

Description

[Yulia Startsev [:yulia] OOO until July 2026]

As part of refactoring ObjectActor to protocol.js, we need to update SymbolIteratorActor. it is instantiated via: 
 -> ObjectActor.enumSymbols

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1451025 - Convert SymbolIteratorActor to protocol.js; .

Review commit: https://reviewboard.mozilla.org/r/234136/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234136/

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Alex, this is my first protocol.js actor refactor so I'm not sure I checked all the boxes. I didn't put anything regarding releasing since there wasn't any existing function and I guess this will be managed when the ObjectActor (which creates the SymbolIteratorActor) use protocol.js as well.

Comment 3

[Yulia Startsev [:yulia] OOO until July 2026]

Comment on attachment 8965389 [details]
Bug 1451025 - Convert SymbolIteratorActor to protocol.js; .

https://reviewboard.mozilla.org/r/234136/#review240010

::: devtools/shared/specs/index.js:200
(Diff revision 1)
>      front: "devtools/shared/fronts/stylesheets",
>    },
>    {
> +    types: ["symbolIterator"],
> +    spec: "devtools/shared/specs/symbol-iterator",
> +    front: null,

i have a question about this! should we leave comments if a front is null? is this an exception? asking because i saw it in relation to breakpoints and it had a comment, and wanted to know also for future refactorings!

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8965389 [details]
Bug 1451025 - Convert SymbolIteratorActor to protocol.js; .

https://reviewboard.mozilla.org/r/234136/#review240868

Looks good to me, thanks for jumping on that :)

Could you push to talos to ensure there is no regression like the one I get for netmonitor?
(1/ I'm not sure we have a test involving symbols? 2/ Same question about iterators? [are they used only on expand?])

::: devtools/server/actors/object/symbol-iterator.js:47
(Diff revision 1)
> -SymbolIteratorActor.prototype = {
> -  actorPrefix: "symbolIterator",
>  
> -  grip() {
> +  form() {
>      return {
> -      type: this.actorPrefix,
> +      type: "symbolIterator",

You should be able to use `this.typeName` instead of hardcoding `"symbolInterator"` here.

::: devtools/shared/specs/index.js:200
(Diff revision 1)
>      front: "devtools/shared/fronts/stylesheets",
>    },
>    {
> +    types: ["symbolIterator"],
> +    spec: "devtools/shared/specs/symbol-iterator",
> +    front: null,

I don't think it is useful to comment against each null front attribute. It only means that there is no front yet for this actor and that we still use old fashion client API for it.
If you think it help we could comment about that once, before the list.

::: devtools/shared/specs/symbol-iterator.js:30
(Diff revision 1)
> +  configurable: "boolean",
> +  enumerable: "boolean",
> +  writable: "nullable:boolean",
> +  value: "nullable:json",
> +  get: "nullable:json",
> +  set: "nullable:json",

The spec is a nice opportunity to document our protocol. protocol.js type system can't describe every detail, like here.
We could drop a word about when/why attributes are nullable.
Looking at:
https://searchfox.org/mozilla-central/source/devtools/server/actors/object.js#541-582
Each field is defined depending on the value definition.
`writable` is only set if there is a final `value`,
otherwise, `get` and/or `set` is defined if there is getters and/or setters.

Also it would be great if we could type value grips with something more explicit than `json`, but it looks challenging given its polymorphism...
That's something we can look into as part of bug 1450944.

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8965389 [details]
Bug 1451025 - Convert SymbolIteratorActor to protocol.js; .

https://reviewboard.mozilla.org/r/234136/#review240868

Indeed it's only used when expanding objects. And the object expand test only does not have any symbols (https://searchfox.org/mozilla-central/rev/b55e1a1cbcaee34878e133fbac20c4c2af6e11b5/testing/talos/talos/tests/devtools/addon/content/tests/webconsole/objectexpand.js#30-37). I think it would make sense to add symbols to the object being expanded in the test, before landing this change so we can check if there's any effects switching to protocol.js.

> The spec is a nice opportunity to document our protocol. protocol.js type system can't describe every detail, like here.
> We could drop a word about when/why attributes are nullable.
> Looking at:
> https://searchfox.org/mozilla-central/source/devtools/server/actors/object.js#541-582
> Each field is defined depending on the value definition.
> `writable` is only set if there is a final `value`,
> otherwise, `get` and/or `set` is defined if there is getters and/or setters.
> 
> Also it would be great if we could type value grips with something more explicit than `json`, but it looks challenging given its polymorphism...
> That's something we can look into as part of bug 1450944.

yulia was asking to me how we could have "union" type, instead of defining every property as nullable, and I couldn't fidn anything in protocol.js for such thing.
Would this work be a good opportunity to add such thing to protocol.js ?

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8965389 [details]
Bug 1451025 - Convert SymbolIteratorActor to protocol.js; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234136/diff/1-2/

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #5)
> yulia was asking to me how we could have "union" type, instead of defining
> every property as nullable, and I couldn't fidn anything in protocol.js for
> such thing.
> Would this work be a good opportunity to add such thing to protocol.js ?

I think a union type would be useful, yes.  We also saw some union types in Alex's netmonitor conversion as well.  For type syntax, I guess something like "boolean|string"?

Seems better to add it in a separate commit / bug from the main work here.  Also you could go with what you have for types here for now, then add the feature after and clean this up once it exists.

Comment 8

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> Seems better to add it in a separate commit / bug from the main work here. 
> Also you could go with what you have for types here for now, then add the
> feature after and clean this up once it exists.

+1, but be careful to not slow down protocol.js even more.
I'm discovering that its powerness has a significant performance cost.
I may start suggesting to simplify it, by removing (almost) unused features or scale down some features.

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

I applied the patch from Bug 1452936 and triggered talos jobs https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=29d62e2398eb34a301034079c21321921ad7849b&newProject=try&newRevision=170d054b3620e3efb25392acdf0a472dd510ef7f&framework=1

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Yes, for the union type I was referring as "current work" as the META to move everything to protocol.js. I'll go on with this patch if damp shows no regression

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #9)
> I applied the patch from Bug 1452936 and triggered talos jobs
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=29d62e2398eb34a301034079c2132192
> 1ad7849b&newProject=try&newRevision=170d054b3620e3efb25392acdf0a472dd510ef7f&
> framework=1

So I'm seeing a < 2% regression on object expand (See [1] & [2]).


[1] https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=29d62e2398eb34a301034079c21321921ad7849b&newProject=try&newRevision=170d054b3620e3efb25392acdf0a472dd510ef7f&originalSignature=d0aa31b4a8617d5bda9bc8c5ca7bfa9a77c38c7c&newSignature=d0aa31b4a8617d5bda9bc8c5ca7bfa9a77c38c7c&filter=object&framework=1
[2] https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=29d62e2398eb34a301034079c21321921ad7849b&newProject=try&newRevision=170d054b3620e3efb25392acdf0a472dd510ef7f&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=objectexpand&framework=1

Comment 12

[Alexandre Poirot [:ochameau]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #11)
> (In reply to Nicolas Chevobbe [:nchevobbe] from comment #9)
> > I applied the patch from Bug 1452936 and triggered talos jobs
> > https://treeherder.mozilla.org/perf.html#/
> > compare?originalProject=try&originalRevision=29d62e2398eb34a301034079c2132192
> > 1ad7849b&newProject=try&newRevision=170d054b3620e3efb25392acdf0a472dd510ef7f&
> > framework=1
> 
> So I'm seeing a < 2% regression on object expand (See [1] & [2]).

Thanks a lot for the DAMP test!
So there is a regression, but small enough.
I imagine it isn't specific to this particular actor.
If I manage to improve protocol.js perf (see bug 1449162), it will most likely also improve this test.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/69bd463e5db0
Convert SymbolIteratorActor to protocol.js; r=ochameau.

Comment 14

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/69bd463e5db0