1231506 - JS::ubi::Node should report source text and its size

Status: NEW

(Core :: JavaScript Engine, defect)

Description

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

We see ScriptSourceObject objects, but they have a refcounted ptr to a ScriptSource which has the actual source text, and we do not report the source text size.

Two options:

1. Specialize JS::ubi::Node::size for ScriptSourceObject (either in JSObject::addSizeOfExcludingThis or via something like bug 1056992) and add size / refcount to the ScriptSourceObject's size

2. Make a JS::ubi::Concrete specialization for ScriptSource so it is a part of ubi::Node graphs.

Comment 1

[Eric Rahm [:erahm]]

Nick, is this still applicable? It looks like we have some sort of script source cache now which probably complicates things (we no longer report the data in our addSizeOfThis function). If they're stored in the cache would we really want something like 'size / (refcount - 1)'?

Comment 2

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

Yes, this is still applicable. Rather than estimating how much of the source text we are responsible for, I think we should create a JS::ubi::Concrete<SharedImmutableString> implementation and have scripts report an edge to their source text. The analyses know how to handle nodes with multiple incoming edges; that's what they're designed for.

Comment 3

[Eric Rahm [:erahm]]

Kris, can you take a look at this?

Comment 4

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

> 11:51 <kriswright> fitzgen: I'm looking at Bug 1231506 , figuring out where to start
> right now. Can you point me to any examples of reporting edges that could help? Thanks
> so much in advance!

A JS::ubi::Node's edges are enumerated by its `edges` method: https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/js/public/UbiNode.h#796

Similar to all other operations, this proxies to the JS::ubi::Node's referent's JS::ubi::Concrete specialization.

https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/js/public/UbiNode.h#81-87
https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/js/public/UbiNode.h#522
https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/js/public/UbiNode.h#649-671

Right now, JSScript uses the GC's tracing APIs to enumerate its edges, by inheriting from TracerConcreteWithCompartment -> TracerConcrete -> Base:

https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/js/public/UbiNode.h#1060
https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/js/src/vm/UbiNode.cpp#322

The `edges` method returns something that implements the `JS::ubi::EdgeRange` interface, which allows callers to access each edge in the range one by one.

https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/js/public/UbiNode.h#885

We can write a `Chain` combinator that takes two `js::UniquePtr<JS::ubi::EdgeRange>`s and implements the `EdgeRange` interface itself. It would yield each edge in the first range, and then once the first range is exhausted, would yield each edge in the second range.

With that in hand, we would override the `JS::ubi::Concrete<JSScript>::edges` method to:

1. call the super implementation to get the GC's reported edge range
2. construct a SimpleEdgeRange containing only the edge to the SharedImmutableString
3. Use the Chain combinator to concatenate these ranges and return them

Comment 5

[Kris Wright :KrisWright]

Attachment: JS::ubi::Node should report source text and its size

WIP - Added SharedImmutableString and SharedImmutableTwoByteString specialization to concrete class
Added edges override to JSScript concrete specialization
Added a way to create standalone edges out of ScriptSource
Created Combine class

Comment 6

[Eric Rahm [:erahm]]

Comment on attachment 8980440 [details] [diff] [review]
JS::ubi::Node should report source text and its size

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

Looks like a great start. I've added some notes and questions that should hopefully help out.

::: js/public/UbiNode.h
@@ +1020,4 @@
>  // JS::TraceChildren.
>  template<typename Referent>
>  class JS_PUBLIC_API(TracerConcrete) : public Base {
> +    JS::Zone* zone() const override;

Is this needed by the ubinode classes?

@@ +1134,4 @@
>      static void construct(void* storage, void* ptr) { new (storage) Concrete(ptr); }
>  };
>  
> +class Combine : EdgeRange {

I think you want public inheritance, ie `public EdgeRange`

@@ +1136,5 @@
>  
> +class Combine : EdgeRange {
> +
> +public:
> +  Combine(js::UniquePtr<EdgeRange>&& parent, Edge toAdd)

Probably want `Edge&& toAdd`

@@ +1137,5 @@
> +class Combine : EdgeRange {
> +
> +public:
> +  Combine(js::UniquePtr<EdgeRange>&& parent, Edge toAdd)
> +  {

One way to do this is to store both the parent and raw edge as members. You'd do something like:

> popFront() {
>  if (!parent.isEmpty())
>    parent.popFront()
>    _front = &parent.front();
>  else if !used_edge
>    used_edge = true
>    front_ = &edge
>  else
>    MOZ_ASSERT(front_)
>    front_ = nullptr
> }

Which isn't super graceful, but hopefully clarifies things a bit. You can also look at PrecomputedEdgeRange [1] as a similar example.

[1] https://searchfox.org/mozilla-central/rev/dc6d85680539cb7e5fe2843d38a30a0581bfefe1/js/public/UbiNode.h#920-941

::: js/src/vm/JSScript.cpp
@@ +1654,5 @@
> +JS::ubi::Edge
> +ScriptSource::sourceEdge()
> +{
> +  JS::ubi::Edge edge;
> +  if (data.is<1>()) {

I think this should be `data.is<Uncompressed>`

@@ +1659,5 @@
> +    // source is uncompressed
> +    JS::ubi::Node n(&data.as<Uncompressed>().string);
> +    edge = JS::ubi::Edge(nullptr, n);
> +    return edge;
> +  } else if (data.is<2>()) {

... and this `is<Compressed>`

@@ +4515,5 @@
> +  UniquePtr<JS::ubi::EdgeRange> parentSet = JS::ubi::TracerConcrete<JSScript>::edges(cx, wantNames);
> +  js::ScriptSource* src = get().scriptSource();
> +  JS::ubi::Edge toAdd = mozilla::Move(src->sourceEdge());
> +  //We then want to use our combinator class to combine the two (the parent and the basic edge)
> +  JS::ubi::Combine(mozilla::Move(parentSet), mozilla::Move(toAdd));

I think the idea is this would be the thing returned, ie:

return UniquePtr<...>(new Combine(...,...))

Comment 7

[Kris Wright :KrisWright]

Attachment: JS::ubi::Node should report source text and its size

WIP - Added SharedImmutableString and SharedImmutableTwoByteString specialization to concrete class
Added edges override to JSScript concrete specialization
Added a way to create standalone edges out of ScriptSource
Created Combine class - crashes when replacing front_ with toAdd.

Comment 8

[Kris Wright :KrisWright]

Attachment: JS::ubi::Node should report source text and its size

WIP - Added SharedImmutableString and SharedImmutableTwoByteString specialization to concrete class
Added edges override to JSScript concrete specialization
Added a way to create standalone edges out of ScriptSource
Created Combine class - crashes when replacing front_ with toAdd.

Comment 9

[Kris Wright :KrisWright]

Attachment: JS::ubi::Node should report source text and its size

Added SharedImmutableString and SharedImmutableTwoByteString specialization to concrete class - inherits from JS::ubi::Base
Added edges override to JSScript concrete specialization
Added a way to create edges out of ScriptSource

Comment 10

[Kris Wright :KrisWright]

Attachment: JS::ubi::Node should report source text and its size

Added SharedImmutableString and SharedImmutableTwoByteString specialization to concrete class - inherits from JS::ubi::Base
Added edges override to JSScript concrete specialization
Added a way to create edges out of ScriptSource

Comment 11

[Kris Wright :KrisWright]

Attachment: JS::ubi::Node should report source text and its size

Added SharedImmutableString and SharedImmutableTwoByteString specialization to concrete class - inherits from JS::ubi::Base
Added edges override to JSScript and LazyScript concrete specialization
Added a way to create edges out of ScriptSource

Comment 12

[Kris Wright :KrisWright]

Attachment: JS::ubi::Node should report source text and its size

Added SharedImmutableString and SharedImmutableTwoByteString specialization to concrete class - inherits from JS::ubi::Base
Added edges override to JSScript and LazyScript concrete specialization
Added a way to create edges out of ScriptSource

Comment 13

[Kris Wright :KrisWright]

Attachment: JS::ubi::Node should report source text and its size

Added SharedImmutableString and SharedImmutableTwoByteString specialization to concrete class - inherits from JS::ubi::Base
Added edges override to JSScript and LazyScript concrete specialization
Added a way to create edges out of ScriptSource

Comment 14

[Kris Wright :KrisWright]

Attachment: JS::ubi::Node should report source text and its size

Added SharedImmutableString and SharedImmutableTwoByteString specialization to concrete class - inherits from JS::ubi::Base
Added edges override to JSScript and LazyScript concrete specialization
Added a way to create edges out of ScriptSource

Comment 15

[Kris Wright :KrisWright]

Attachment: JS::ubi::Node should report source text and its size

Added SharedImmutableString and SharedImmutableTwoByteString specialization to concrete class - inherits from JS::ubi::Base
Added edges override to JSScript and LazyScript concrete specialization
Added a way to create edges out of ScriptSource

Comment 16

[Kris Wright :KrisWright]

Attachment: JS::ubi::Node should report source text and its size

Added SharedImmutableString and SharedImmutableTwoByteString specialization to concrete class - inherits from JS::ubi::Base
Added edges override to JSScript and LazyScript concrete specialization
Added a way to create edges out of ScriptSource
Adjusted the way coarsetype::strings are presented in tree map to display SharedImmutableStrings and JSStrings separately.

Comment 17

[Kris Wright :KrisWright]

Attachment: JS::ubi::Node should report source text and its size

Added SharedImmutableString and SharedImmutableTwoByteString specialization to concrete class - inherits from JS::ubi::Base
Added edges override to JSScript and LazyScript concrete specialization
Added a way to create edges out of ScriptSource
Adjusted the way coarsetype::strings are presented in tree map to display SharedImmutableStrings and JSStrings separately.

Comment 18

[Kris Wright :KrisWright]

Attachment: JS::ubi::Node should report source text and its size

Added SharedImmutableString and SharedImmutableTwoByteString specialization to concrete class - inherits from JS::ubi::Base
Added edges override to JSScript and LazyScript concrete specialization
Added a way to create edges out of ScriptSource
Adjusted the way coarsetype::strings are presented in tree map to display SharedImmutableStrings and JSStrings separately.
Added a new function and a variant matcher that generates a ubi::Node and an edge for it. To respect the wantNames bit of JS::ubi::Base::edges, we first get a node and then attach a name to the edge that holds it.

Comment 19

[Kris Wright :KrisWright]

Clearing assignment on some old bugs.