1359027 - Wasm: Split tier-invariant and tier-variant parts of wasm data and code apart

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Lars T Hansen [:lth]]

Following on the work to share code:

Tier-variant parts of Metadata should hang off Code and be uniquely owned by it, if possible.

Tier-invariant parts of Metadata should be in some refcounted structure, most likely, and might be referenced by the Tier-variant object.

Comment 1

[Lars T Hansen [:lth]]

I think what I'd like to do here is to create a new structure, CommonMetadata, containing the tier-invariant parts of the current Metadata.  This would be subclassed by AsmJSMetaData as Metadata is subclassed now.

Then the structure Metadata would carry the tier-variant parts of the current Metadata, and a link 'common' pointing to a shared CommonMetadata object.  It would provide a facade that forwards to methods on 'common' when appropriate.  For data fields, I don't know if it's simpler to provide methods to simplify access to 'common' or for clients to just go to that object; the current style favors the latter.

Clients would be provided with a reference to the new Metadata object.

Comment 2

[Lars T Hansen [:lth]]

Attachment: bug1277562-separate-tiers.patch

WIP.  This attempts to clearly separate all tier-variant data from tier-invariant data and make all code tier-aware.  Tiering is not implemented, per se, and there are some FIXMEs here for where tiers are not easily resolvable right now (import/export), but the overall structure might be similar to this.

Specifically,

- Metadata is split into tier-invariant and tier-variant parts
- LinkData ditto (the tier-invariant part is split out as GlobalData)
- there are new SharedTieredCode and SharedTieredLinkData structures that
  support two tiers, and which require you to be specific about which tier
  you want (the shared LinkData could probably be uniquely owned; later)
- all consumers of metadata now either request the tier-invariant data or
  state the tier they want
- all consumers of code and linkdata state the tier they want

One thing I don't know if we need here anymore is that the tier-variant Metadata points to the tier-invariant ditto, and (especially) that it has accessors that forward to the tier-invariant structure - code that wants to access those fields should explicitly ask for the tier-invariant data instead.  For now it stays, though.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 8867702 [details] [diff] [review]
bug1277562-separate-tiers.patch

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

Great; looks like this is heading in the right direction.  I was thinking, for getting an initial landable patch, it is kindof hard to review -- and avoid causing conflicts after landing -- when there's a lot of partially-implemented tiering logic.  Could the first landable patch contains *just* the central (mostly mechanical) changes suggested below to WasmCode.h and WasmCompartment.h, without yet speaking about multiple tiers?

::: js/src/wasm/WasmCode.h
@@ +349,5 @@
> +
> +    ModuleKind kind() const { return kind_; }
> +    ModuleKind& kind() { return kind_; }
> +    MemoryUsage memoryUsage() const { return memoryUsage_; }
> +    MemoryUsage& memoryUsage() { return memoryUsage_; }

This is indeed quite a lot of accessors, providing little encapsulative value (given that metadata is const).  I think you mentioned wanting to remove these and explicitly deal with Common/Tiered types and that makes sense to me.  In that case, perhaps the tier-variant Metadata don't need to point to the tier-invariant Metadata; rather the top-level struct owns (tier-variant-1, tier-variant-2, tier-invariant)?

@@ +587,4 @@
>  typedef RefPtr<const Code> SharedCode;
>  typedef RefPtr<Code> MutableCode;
>  
> +class TieredCode : public ShareableBase<TieredCode>

Since the file is named WasmCode.h, it feels nice to me to have the "root" class be wasm::Code, rather than wasm::TieredCode.  This naming makes the tiering more an internal detail of wasm::Code.  Perhaps, then, the tier-variant class would be called wasm::CodeTier.  For symmetry then we could have tier-invariant wasm::Metadata (by above comment, I think owned by wasm::Code) and wasm::MetadataTier, owned by wasm::CodeTier.  How does that sound?

@@ +588,5 @@
>  typedef RefPtr<Code> MutableCode;
>  
> +class TieredCode : public ShareableBase<TieredCode>
> +{
> +    SharedCode code_;

Can the individual tiers be uniquely owned?

::: js/src/wasm/WasmCompartment.h
@@ +42,5 @@
> +    // only those instances that also have code at the second tier, and is
> +    // sorted by the base address of the second tier code.
> +
> +    InstanceVector instances0_;
> +    InstanceVector instances1_;

wasm::Compartment has two general purposes: providing the list of all instances for debugging/profiling in a compartment, and lookupCode().  I think the former purpose should be unaffected by tiering.  For the latter, I think unfortunately we need to create a process-wide R/W-locked map.  On the bright side, since it's process-wide, we can simply register code immediately upon creation (say in CodeSegment::create).  Since it's all about wasm::Code, this code could be moved from WasmCompartment.* to WasmCode.*, changing wasm::Compartment::lookupCode into wasm::LookupCode().

Comment 4

[Lars T Hansen [:lth]]

(In reply to Luke Wagner [:luke] from comment #3)
> Comment on attachment 8867702 [details] [diff] [review]
> bug1277562-separate-tiers.patch
> 
> Review of attachment 8867702 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great; looks like this is heading in the right direction.  I was thinking,
> for getting an initial landable patch, it is kindof hard to review -- and
> avoid causing conflicts after landing -- when there's a lot of
> partially-implemented tiering logic.  Could the first landable patch
> contains *just* the central (mostly mechanical) changes suggested below to
> WasmCode.h and WasmCompartment.h, without yet speaking about multiple tiers?

I'll see what I can do, but if it means undoing a lot of work then probably not.

> ::: js/src/wasm/WasmCode.h
> @@ +349,5 @@
> > +
> > +    ModuleKind kind() const { return kind_; }
> > +    ModuleKind& kind() { return kind_; }
> > +    MemoryUsage memoryUsage() const { return memoryUsage_; }
> > +    MemoryUsage& memoryUsage() { return memoryUsage_; }
> 
> This is indeed quite a lot of accessors, providing little encapsulative
> value (given that metadata is const).  

Metadata is not actually const.  It is const most of the time, but it is not const during module generation, and that's why these are here, and the fact that there are accessors is just so that we access all fields in the same manner (ie reduced mental strain).

> I think you mentioned wanting to
> remove these and explicitly deal with Common/Tiered types and that makes
> sense to me.  In that case, perhaps the tier-variant Metadata don't need to
> point to the tier-invariant Metadata; rather the top-level struct owns
> (tier-variant-1, tier-variant-2, tier-invariant)?

I think so, possibly with some kind of encapsulation to avoid having to deal with lots of variables (not sure yet).

> 
> @@ +587,4 @@
> >  typedef RefPtr<const Code> SharedCode;
> >  typedef RefPtr<Code> MutableCode;
> >  
> > +class TieredCode : public ShareableBase<TieredCode>
> 
> Since the file is named WasmCode.h, it feels nice to me to have the "root"
> class be wasm::Code, rather than wasm::TieredCode.  This naming makes the
> tiering more an internal detail of wasm::Code.  Perhaps, then, the
> tier-variant class would be called wasm::CodeTier.

IIUC, you are suggesting renaming Code => CodeTier and TieredCode => Code but otherwise keeping the proposed structure?

> For symmetry then we
> could have tier-invariant wasm::Metadata (by above comment, I think owned by
> wasm::Code) and wasm::MetadataTier, owned by wasm::CodeTier.  How does that
> sound?

Metadata I'm not sure about yet, though I expect you're right.  But it would be ditto for LinkData.

> 
> @@ +588,5 @@
> >  typedef RefPtr<Code> MutableCode;
> >  
> > +class TieredCode : public ShareableBase<TieredCode>
> > +{
> > +    SharedCode code_;
> 
> Can the individual tiers be uniquely owned?

Probably, since Module and Instance will deal in the blob containing both tiers.

> ::: js/src/wasm/WasmCompartment.h
> @@ +42,5 @@
> > +    // only those instances that also have code at the second tier, and is
> > +    // sorted by the base address of the second tier code.
> > +
> > +    InstanceVector instances0_;
> > +    InstanceVector instances1_;
> 
> wasm::Compartment has two general purposes: providing the list of all
> instances for debugging/profiling in a compartment, and lookupCode().  I
> think the former purpose should be unaffected by tiering.  For the latter, I
> think unfortunately we need to create a process-wide R/W-locked map.  On the
> bright side, since it's process-wide, we can simply register code
> immediately upon creation (say in CodeSegment::create).  Since it's all
> about wasm::Code, this code could be moved from WasmCompartment.* to
> WasmCode.*, changing wasm::Compartment::lookupCode into wasm::LookupCode().

OK, I'll try to undo these changes here, and then we can discuss that structure more in detail (and in connection with the patch in bug 1365069).

Comment 5

[Luke Wagner [:luke]]

(In reply to Lars T Hansen [:lth] from comment #4)
> > This is indeed quite a lot of accessors, providing little encapsulative
> > value (given that metadata is const).  
> 
> Metadata is not actually const.  It is const most of the time, but it is not
> const during module generation, and that's why these are here, and the fact
> that there are accessors is just so that we access all fields in the same
> manner (ie reduced mental strain).

Right, but during module generation, Metadata is only accessible by the ModuleGenerator generator itself, which needs full access to arbitrarily mutate Metadata.  So if we did want to put everything behind accessors, I think the way to do it would be (1) make all fields private, (2) only expose 'const' accessor methods, (3) have Metadata friend ModuleGenerator so that ModuleGenerator does not pollute the public interface with tons of mutators.

> IIUC, you are suggesting renaming Code => CodeTier and TieredCode => Code
> but otherwise keeping the proposed structure?

Yup

> > For symmetry then we
> > could have tier-invariant wasm::Metadata (by above comment, I think owned by
> > wasm::Code) and wasm::MetadataTier, owned by wasm::CodeTier.  How does that
> > sound?
> 
> Metadata I'm not sure about yet, though I expect you're right.  But it would
> be ditto for LinkData.

Right, LinkDataTier.  I think "CodeTier" sounds nice, but "MetadataTier" and "LinkDataTier" not as much (other than being symmetric with "CodeTier"); so happy to consider other naming suggestions (MetadataForTier/LinkDataForTier?).  Mostly I just like having wasm::Code being the name of the top-level shared-by-modules-and-instances code object.

On an unrelated note: if you need to get the whole patch up-and-running before bug 1351488 is finished, you could have Module::serialize just condvar.wait on the ion helper thread task.  It'll happen on the main thread, so we probably wouldn't want to land, but it would be sufficient to get everything green/tested.

Comment 6

[Lars T Hansen [:lth]]

Attachment: bug1359027-split-metadata.patch

In the spirit of reviewable patches, this one only splits Metadata, and it hangs the tier-specific data off the tier-invariant data.  I removed all accessor functions.  There is now an accessor, called simply tier() on the Metadata structure and metadataTier() everywhere else, that grabs the only tier we have but will later morph to metadata(Tier), most likely.  In any case we clearly seprate tier-invariant and tier-variant data, which is the main purpose here.

I decided to go with "MetadataTier" for the structure name; it doesn't roll off the tongue but it's plain what it represents.

Anyway, with this structure, we're back to the situation where most metadata accesses do not change vis-a-vis the existing code, so the patch is pretty simple after all.

(More patches for LinkData and Code coming as I have them.)

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8868211 [details] [diff] [review]
bug1359027-split-metadata.patch

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

Great, thanks!

::: js/src/wasm/WasmCode.cpp
@@ +459,5 @@
>             sizeof(hash);
>  }
>  
> +size_t
> +MetadataTier::serializedSize() const

nit: could you move the other three MetadataTier serialization methods up here, before the Metadata methods.

::: js/src/wasm/WasmCode.h
@@ +351,5 @@
>  struct Metadata : ShareableBase<Metadata>, MetadataCacheablePod
>  {
> +    // Both `tier_` and the means of accessing it will become more complicated
> +    // when tiering is implemented.
> +    MutableMetadataTier tier_;

I know ModuleGenerator stores a MutableMetadataTier, but I think it could instead simply store a *.  Could MetadataTier then be non-ShareableBase and this made a UniqueConstMetadataTier?

@@ +356,5 @@
> +
> +    const MetadataTier& tier() const { return *tier_; }
> +    MetadataTier& tier() { return *tier_; }
> +
> +    explicit Metadata(MetadataTier* tier, ModuleKind kind = ModuleKind::Wasm)

... then this would change to a UniqueConstMetadataTier.

Btw, I learned that move-constructors do implicit upcasts so you can pass a Move() of a UniqueMetadataTier to a UniqueConstMetadataTier without any casting.

Comment 8

[Lars T Hansen [:lth]]

(In reply to Luke Wagner [:luke] from comment #7)
> Comment on attachment 8868211 [details] [diff] [review]
> bug1359027-split-metadata.patch
> 
> Review of attachment 8868211 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/wasm/WasmCode.h
> @@ +351,5 @@
> >  struct Metadata : ShareableBase<Metadata>, MetadataCacheablePod
> >  {
> > +    // Both `tier_` and the means of accessing it will become more complicated
> > +    // when tiering is implemented.
> > +    MutableMetadataTier tier_;
> 
> I know ModuleGenerator stores a MutableMetadataTier, but I think it could
> instead simply store a *.

Yeah, we could do that.

> Could MetadataTier then be non-ShareableBase and this made a UniqueConstMetadataTier?

That depends on what the structure for Code ends up being - whether the MetadataTier will be referenced only from Metadata, or also from the CodeTier to which it belongs.  So I will keep it shared for now, but we'll keep an eye on it.

Comment 9

[Lars T Hansen [:lth]]

Attachment: bug1359027-split-metadata-v2.patch

Patch updated to address (some) review comments.  Carrying Luke's r+.

Comment 10

[Lars T Hansen [:lth]]

Attachment: bug1359027-uniquely-owned-metadata.patch

First addendum to the metadata patch: Make the tiered data uniquely owned, more or less as proposed.  It's not const, because I could not make that work without inserting some const_casts elsewhere, and it didn't seem worth the bother mucking around with it more.

Comment 11

[Lars T Hansen [:lth]]

Attachment: bug1359027-funcexport-funcimport-tiered.patch

Addendum 2 to the metadata patch: funcImports and funcExports contain code offsets and are therefore also tier-variant.

Comment 12

[Lars T Hansen [:lth]]

Attachment: bug1359027-split-code.patch

Split the tier-variant parts of code from the tier-invariant parts.  Here I've gone the easy route: the tier-variant parts comprise just the CodeSegment, so this code amounts to renaming some accessors, provided we do not link to the tier-variant metadata from the tier-variant code.  At the present time there was no need to do that, hence no introduction here of "CodeTier".  Doing so later won't create many waves, anyhow.

Comment 13

[Lars T Hansen [:lth]]

Attachment: bug1359027-split-linkdata.patch

Split the tier-variant parts of LinkData from the tier-invariant parts, using the pattern established by Metadata and Code.

Comment 14

[Lars T Hansen [:lth]]

Attachment: bug1359027-tag-tiered-data.patch

Tag MetadataTier, CodeSegment, and LinkDataTier with the type of code they contain (the CompileMode), for self-identification purposes.  This patch could land either as this bug or as part of bug 1277562, but I've put it here because it rounds out the work on splitting data.

Comment 15

[Luke Wagner [:luke]]

Comment on attachment 8869002 [details] [diff] [review]
bug1359027-uniquely-owned-metadata.patch

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

::: js/src/wasm/AsmJS.cpp
@@ +1775,4 @@
>  
>    public:
>      bool init() {
> +        UniqueMetadataTier tierMetadata = js::MakeUnique<MetadataTier>();

here and below x2: I feel like 'auto' is appropriate, and not discarding useful type info, when the right-hand-side explicitly states the type being allocated.

Comment 16

[Luke Wagner [:luke]]

Comment on attachment 8869003 [details] [diff] [review]
bug1359027-funcexport-funcimport-tiered.patch

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

::: js/src/wasm/WasmCode.cpp
@@ +572,2 @@
>  
> +    return tier().funcExports[match];

nit: given 3 repetitions of tier().funcExports (which will grow more lengthy when we have multiple tiers), I'd hoist a & named funcExports.

Comment 17

[Luke Wagner [:luke]]

Comment on attachment 8869005 [details] [diff] [review]
bug1359027-split-code.patch

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

Makes sense; nice to have less concepts.  Actually, if we later need to introduce fields (that might've gone in wasm::CodeTier), they could just go in CodeSegment.

Comment 18

[Luke Wagner [:luke]]

Comment on attachment 8869006 [details] [diff] [review]
bug1359027-split-linkdata.patch

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

::: js/src/wasm/WasmModule.h
@@ +76,5 @@
> +typedef UniquePtr<LinkDataTier> UniqueLinkDataTier;
> +
> +struct LinkDataCacheablePod
> +{
> +    uint32_t globalDataLength;

Could you instead move this field to MetadataCacheablePod?

Comment 19

[Luke Wagner [:luke]]

Comment on attachment 8869007 [details] [diff] [review]
bug1359027-tag-tiered-data.patch

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

::: js/src/wasm/AsmJS.cpp
@@ +1775,4 @@
>  
>    public:
>      bool init() {
> +        UniqueMetadataTier tierMetadata = js::MakeUnique<MetadataTier>(CompileMode::Ion);

ditto 'auto' point in previous patch here and below

::: js/src/wasm/WasmCode.h
@@ +345,4 @@
>  
>  struct MetadataTier
>  {
> +    MetadataTier(CompileMode mode) : mode(mode) {}

'explicit' (probably not worth making MOZ_IMPLICIT)

Comment 20

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a306daf50c5516c50af6f59f048354a1d57145ab

Comment 21

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2bb444c525c273667f2c13cd1445cc44740df68f
Bug 1359027 - Split tier-invariant and tier-variant parts of wasm Metadata apart. r=luke

https://hg.mozilla.org/integration/mozilla-inbound/rev/5187ecd492fc7f98e43d15ef1256946a820e5327
Bug 1359027 - Make MetadataTier uniquely owned. r=luke

https://hg.mozilla.org/integration/mozilla-inbound/rev/2bad73fce12b75d5e495409a48377dce07b2f9d3
Bug 1359027 - Move funcExports and funcImports to tiered Metadata. r=luke

https://hg.mozilla.org/integration/mozilla-inbound/rev/22e4abf1e4e7e330fc9eb855f5a2e4ace44774f3
Bug 1359027 - Split tier-invariant and tier-variant parts of wasm Code apart. r=luke

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3e049afffcc2f87cfea98c3a197cade1a07b4fa
Bug 1359027 - Split tier-invariant and tier-variant parts of wasm LinkData apart. r=luke

https://hg.mozilla.org/integration/mozilla-inbound/rev/b644b0a89fab8cd70fbe022c77d7923c1eb142b1
Bug 1359027 - Make CodeSegment, MetadataTier, and LinkDataTier self-identifying. r=luke

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2bb444c525c2
https://hg.mozilla.org/mozilla-central/rev/5187ecd492fc
https://hg.mozilla.org/mozilla-central/rev/2bad73fce12b
https://hg.mozilla.org/mozilla-central/rev/22e4abf1e4e7
https://hg.mozilla.org/mozilla-central/rev/b3e049afffcc
https://hg.mozilla.org/mozilla-central/rev/b644b0a89fab