1891182 - Refactor ModuleGenerator and ModuleEnvironment to make them ready for partial tiering

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement, P3)

Description

[Julian Seward [:jseward]]

Refactor ModuleGenerator and ModuleEnvironment to make them ready for partial
tiering:

• ModuleEnvironment is split and renamed into ModuleMetadata and
  ModuleCodeMetadata

• ModuleMetadata holds a reference to ModuleCodeMetadata
  since MCM is a subset of MM

• ModuleGenerator/OpIter/compilers use ModuleCodeMetadata primarily

• wasm::Module holds a ModuleMetadata

• wasm::Code holds a ModuleCodeMetadata

• ModuleGenerator just becomes the function compilation manager

Comment 1

[Julian Seward [:jseward]]

Attachment: bug1891182-01-rename-ModuleEnv-to-ModuleMeta.diff-1

Renames ModuleEnvironment to ModuleMetadata. No other changes.

Comment 2

[Julian Seward [:jseward]]

Attachment: bug1891182-02-tidy-up-WMA-WMA.diff-1

Removes a WasmMacroAssembler::WasmMacroAssembler constructor that takes a
ModuleMetadata argument which is unused; and when that argument is unused then
the constructor becomes a duplicate of another one, so the whole constructor
is removed.

Comment 3

[Julian Seward [:jseward]]

Base revision is 734540:e21f6f5c4e95.

Comment 4

[Julian Seward [:jseward]]

Attachment: bug1891182-05-rn-DataSegmentEnv.diff-1

Renames DataSegmentEnv to DataSegmentSummary and CustomSectionEnv to
CustomSectionSummary.

Comment 5

[Julian Seward [:jseward]]

Attachment: bug1891182-06-split-out-layout.diff-1

Moves instance data layout into its own method,
ModuleMetadata::doInstanceLayout. (note, ModuleMetadata was formerly known as
ModuleEnvironment).

Comment 6

[Julian Seward [:jseward]]

Attachment: bug1891182-07-create-ModCodeMeta.diff-3

This patch splits out a new structure from ModuleMetadata (previously known as
ModuleEnvironment). The new structure is ModuleCodeMetadata. What remains in
ModuleMetadata is (I think) information only needed for instantiation.
ModuleCodeMetadata carries everything else (that was formerly in
ModuleEnvironment).

ModuleMetadata has been completely removed from the two compilers
(Wasm{Baseline,Ion}Compile.cpp) and WasmInitExpr.cpp, as planned;
those all now work using ModuleCodeMetadata instead.

The patch is huge, but almost all of it is renaming of innumerable variables
and struct fields from "meta" to "codeMeta" (or something consistent).

Comment 7

[Julian Seward [:jseward]]

Attachment: bug1891182-08-Metadata-to-new-file.diff-1

This patch moves ModuleCodeMetadata into its own new file, WasmMetadata.h.
Having it in WasmValidate.h is not very logical and will become increasingly
nonsensical in future since ModuleCodeMetadata is really "all metadata that
has a lifetime equivalent to that of wasm::Code".

It also moves wasm::SectionRange into a new leaf header, WasmBinaryTypes.h.
This is necessary for the above move. It generally allows WasmMetadata.h to
rely on fewer other includes.

Comment 8

[Julian Seward [:jseward]]

Attachment: bug1891182-09-move-stuff-out-of-Metadata.diff-4

This WIP patch moves fields out of wasm::Metadata and parent
wasm::MetadataCacheablePod into WasmCodeMetadata. MetadataCacheablePod has
been completely removed and there remain only 5 fields in Metadata.

Comment 9

[Julian Seward [:jseward]]

Attachment: bug1891182-09-move-stuff-out-of-Metadata.diff-6

(This is a complete version of the patch in comment 8)

Minor changes:

• This changes ModuleCodeMetadata to be an atomic refcounted thing. Do we
  need to change all appearances of ModuleCodeMetadata* to
  RefPtr<ModuleCodeMetadata> now? What about ModuleCodeMetadata& ?

Major changes:

• All fields have been moved out of Metadata (and MetadataCacheablePod)
  into ModuleCodeMetadata, and duplicates in ModuleMetadata have been
  removed. Metadata itself still remains because it is used as an
  inheritance base for AsmJSMetadata. A subsequent patch will remove it.

Comment 10

[Julian Seward [:jseward]]

Attachment: bug1891182-rollup-2024May15-rev8.tar

Rollup of the patches so far, rebased to
738555:2dfb8db5d717 (Tue May 14 02:34:03 2024 +0300).

This obsoletes all previous patches. Each patch now has at least some
semblance of a reasonable commit message, that explains what it does.

Note that the -03 and -04 entries in the series are missing; this is
deliberate. They tried to remove DataSegmentEnv/CustomSegmentEnv, but
had problems. Tarball contents:

 352853 2024-05-14 13:34 tmptmp_bug1891182-01-rn-ModuleEnv-to-ModuleMeta.diff-8
   7767 2024-05-14 13:35 tmptmp_bug1891182-02-tidy-up-WMA-WMA.diff-8
  15139 2024-05-14 14:07 tmptmp_bug1891182-05-rn-DataSegmentEnv.diff-8
  13166 2024-05-14 14:23 tmptmp_bug1891182-06-split-out-layout.diff-8
 346244 2024-05-15 09:01 tmptmp_bug1891182-07-create-ModCodeMeta.diff-8
  37202 2024-05-15 09:52 tmptmp_bug1891182-08-Metadata-to-new-file.diff-8
 197993 2024-05-15 15:50 tmptmp_bug1891182-09-move-stuff-out-of-Metadata.diff-8
  48856 2024-05-15 17:31 tmptmp_bug1891182-10-remove-Metadata.diff-8
  75074 2024-05-15 17:33 tmptmp_bug1891182-11-rn-Metadata.diff-8

Comment 11

[Julian Seward [:jseward]]

Attachment: bug1891182-rollup-2024May17-rev9.tar

Rollup of the patches so far, still based on 738555:2dfb8db5d717.
Changes compared to comment 10 set:

• fixes for 'try'; up to and including #12 run clean on try.

• two new patches (#12, #13) which

  • remove from Module, two fields that have duplicates in ModuleMetadata
  • remove ModuleMetadata from ModuleGenerator

This patch set is pretty much a complete first pass for the "Step 1: Refactor
ModuleGenerator and ModuleEnv/Metadata so it's ready for partial tiering" task.

 352853 2024-05-17 13:00 bug1891182-01-rn-ModuleEnv-to-ModuleMeta.diff-9
   7767 2024-05-17 13:00 bug1891182-02-tidy-up-WMA-WMA.diff-9
  15139 2024-05-17 13:00 bug1891182-05-rn-DataSegmentEnv.diff-9
  13166 2024-05-17 13:00 bug1891182-06-split-out-layout.diff-9
 348874 2024-05-17 13:00 bug1891182-07-create-ModCodeMeta.diff-9
  37918 2024-05-17 13:00 bug1891182-08-Metadata-to-new-file.diff-9
 200757 2024-05-17 13:00 bug1891182-09-move-stuff-out-of-Metadata.diff-9
  48856 2024-05-17 13:00 bug1891182-10-remove-Metadata.diff-9
  75074 2024-05-17 13:01 bug1891182-11-rn-Metadata.diff-9
  61727 2024-05-17 13:01 bug1891182-12-move-stuff-out-of-Module.diff-9
  23843 2024-05-17 13:01 bug1891182-13-remove-ModMeta-from-ModGen.diff-9

Comment 12

[Ryan Hunt [:rhunt]]

(In reply to Julian Seward [:jseward] from comment #11)

Created attachment 9402439 [details]
bug1891182-rollup-2024May17-rev9.tar

Rollup of the patches so far, still based on 738555:2dfb8db5d717.
Changes compared to comment 10 set:

• fixes for 'try'; up to and including #12 run clean on try.

• two new patches (#12, #13) which

  • remove from Module, two fields that have duplicates in ModuleMetadata
  • remove ModuleMetadata from ModuleGenerator

This patch set is pretty much a complete first pass for the "Step 1: Refactor
ModuleGenerator and ModuleEnv/Metadata so it's ready for partial tiering" task.

 352853 2024-05-17 13:00 bug1891182-01-rn-ModuleEnv-to-ModuleMeta.diff-9
   7767 2024-05-17 13:00 bug1891182-02-tidy-up-WMA-WMA.diff-9
  15139 2024-05-17 13:00 bug1891182-05-rn-DataSegmentEnv.diff-9
  13166 2024-05-17 13:00 bug1891182-06-split-out-layout.diff-9
 348874 2024-05-17 13:00 bug1891182-07-create-ModCodeMeta.diff-9
  37918 2024-05-17 13:00 bug1891182-08-Metadata-to-new-file.diff-9
 200757 2024-05-17 13:00 bug1891182-09-move-stuff-out-of-Metadata.diff-9
  48856 2024-05-17 13:00 bug1891182-10-remove-Metadata.diff-9
  75074 2024-05-17 13:01 bug1891182-11-rn-Metadata.diff-9
  61727 2024-05-17 13:01 bug1891182-12-move-stuff-out-of-Module.diff-9
  23843 2024-05-17 13:01 bug1891182-13-remove-ModMeta-from-ModGen.diff-9

Would you want to upload these to phabricator for review and tag me on them?

Comment 13

[Julian Seward [:jseward]]

Attachment: Bug 1891182 - part 01 - rename ModuleEnvironment to ModuleMetadata. r=rhunt.

This patch is huge, but it does only one thing: renames ModuleEnvironment to
ModuleMetadata, and consistently renames variables etc of that type to
moduleMeta / moduleMeta_ / moduleMeta(). No functional change.

This is intended to be a preliminary to splitting out a new type,
CodeMetadata, and associated codeMeta names, in a later patch in this
series.

Comment 14

[Julian Seward [:jseward]]

Attachment: Bug 1891182 - part 02 - tidy up WasmMacroAssembler::WasmMacroAssembler. r=rhunt.

An opportunistic minor cleanup: this patch removes a
WasmMacroAssembler::WasmMacroAssembler constructor that takes a
wasm::ModuleMetadata& for no obvious reason. The entire constructor seems to
be unnecessary and so has been removed.

Comment 15

[Julian Seward [:jseward]]

Attachment: Bug 1891182 - part 05 - rename DataSegmentEnv/CustomSectionEnv from *Env to *Range. r=rhunt.

This patch renames DataSegmentEnv, which is a bit of a vague name -- it's
unclear what "Env" signifies -- to DataSegmentSummary, which is what it is
really. Similarly CustomSectionEnv is renamed to CustomSectionSummary. No
functional change.

Comment 16

[Julian Seward [:jseward]]

Attachment: Bug 1891182 - part 06 - move instance layout code to ModuleMetadata::doInstanceLayout. r=rhunt.

This patch extracts the Instance layout code from ModuleGenerator::init,
::allocateInstanceDataBytesN and allocateInstanceDataBytes, and moves it to a
new method, ModuleMetadata::doInstanceLayout, which is a better place for it.
The code is also somewhat simpler, although functionally identical.

Comment 17

[Julian Seward [:jseward]]

Attachment: Bug 1891182 - part 07 - create CodeMetadata by splitting ModuleMetadata. r=rhunt.

This patch splits out a new structure from ModuleMetadata (previously known as
ModuleEnvironment). The new structure is CodeMetadata. What remains in
ModuleMetadata is (I think) information only needed for instantiation.
CodeMetadata carries everything else (that was formerly in ModuleMetadata).

ModuleMetadata has been completely removed from the two compilers
(Wasm{Baseline,Ion}Compile.cpp) and WasmInitExpr.cpp, as planned;
those all now work using CodeMetadata instead.

The patch is huge, but almost all of it is either:

• Renaming of many type names, struct fields and variables from from
  "{M,m}oduleMeta*" to "{C,c}odeMeta*".

• Plumbing of both CodeMetadata and ModuleMetadata to places that require
  both. This is relatively rare since most routines require only one or the
  other.

Comment 18

[Julian Seward [:jseward]]

Attachment: Bug 1891182 - part 08 - Move ModuleMetadata and CodeMetadata to new file WasmMetadata.h. r=rhunt.

This patch moves both ModuleMetadata and CodeMetadata into a new file,
WasmMetadata.h. They contain far more info than merely what is needed for
validation, so having them in WasmValidate.h pollutes it with non-validation
artefacts.

This patch also moves wasm::SectionRange into a new leaf header,
WasmBinaryTypes.h. This is necessary for the above move. It generally allows
WasmMetadata.h to rely on fewer other includes.

Comment 19

[Julian Seward [:jseward]]

Attachment: Bug 1891182 - part 09 - Move fields out of Metadata into CodeMetadata. r=rhunt.

Minor changes:

This changes CodeMetadata to be an atomic refcounted thing.
Do we need to change all appearances of CodeMetadata* to
RefPtr<CodeMetadata> now? What about CodeMetadata& ?

Major changes:

All fields have been moved out of Metadata (and MetadataCacheablePod) into
CodeMetadata, and duplicates in ModuleMetadata have been removed.
Metadata itself still remains because it is used as an inheritance base for
AsmJSMetadata. A subsequent patch will remove it.

Comment 20

[Julian Seward [:jseward]]

Attachment: Bug 1891182 - part 10 - remove Metadata and make AsmJSMetadata standalone. r=rhunt.

The #09 patch removed all wasm-specific fields from wasm::Metadata, but did
not remove wasm::Metadata itself, because it is inherited from by
AsmJSMetadata, and used to provide different behaviour for wasm vs asm.js in a
few obscure cases related to the profiler.

This patch restricts wasm::Metadata to be an abstract class that provides
access to (is the pure virtual base class of) AsmJSMetadata. wasm::Metadata is
removed from WasmCode.h and instead reappears in AsmJS.h in pure virtual form.
Any place that previously took a Metadata& now takes takes a Metadata*, and
that is non-null only in the case when we are compiling asm.js.

The effect is to restrict wasm::Metadata and js::AsmJSMetadata to providing
support for asm.js compilation only. The next patch in the series (#11)
completes the transformation by renaming those two types appropriately.

Comment 21

[Julian Seward [:jseward]]

Attachment: Bug 1891182 - part 11 - rename what remains of Metadata to CodeMetadataForAsmJS. r=rhunt.

This patch

• renames Metadata to CodeMetadataForAsmJS, since that's what it is.

• renames AsmJSMetadata to CodeMetadataForAsmJSImpl, which still inherits from
  (now virtual) CodeMetadataForAsmJS, so as to avoid having to expose
  CodeMetadataForAsmJSImpl details in AsmJS.h.

• In WasmGenerator and related classes, renames metadata_ to
  codeMetaForAsmJS_, since that's what it is. This is a supplemental
  field to codeMeta_, which, when non-null, indicates that we're compiling
  asm.js; when null, indicates that we're compiling wasm.

There are no other (non-renaming) changes.

Comment 22

[Julian Seward [:jseward]]

Attachment: Bug 1891182 - part 12 - remove Module::{imports,exports} because they are duplicated in ModuleMetadata. r=rhunt.

This patch

• changes ModuleMetadata into a refcounted thing

• gives Module a reference to ModuleMetadata

• removes Module::{imports, exports} and uses the originals in ModuleMetadata
  instead

Comment 23

[Julian Seward [:jseward]]

Attachment: Bug 1891182 - part 13 - remove ModuleMetadata from ModuleGenerator. r=rhunt.

This patch

• Removes ModuleMetadata from ModuleGenerator. Instead, ModuleMetadata is to
  be passed directly to ModuleGenerator::finishModule as required.

• (ridealong) simplifies the return convention for
  ModuleGenerator::finishCodeMetadata

Comment 26

[Julian Seward [:jseward]]

Attachment: Bug 1891182 - part 14 - address previous review comments. r=rhunt.

This patch addresses review comments in previous patches in this stack,
specifically the following parts: 01 02 05 06 07 08 09 10 11 12 and 13.

The changes are as follows (more or less in the order in which they appear in
the patch):

• merge CodeMetadataForAsmJSImplCacheablePod into CodeMetadataForAsmJSImpl

• turn GetFuncNameForWasm into a method of CodeMetadata

• remove misc unneeded includes

• update or remove various comments marked FIXME

• resolve partial mutual redundancy of CM::builtinModules vs CM::features by
  removing the former and serializing all of the latter

• instance layout: more fully integrate it into CodeMetadata, and rename root
  method to CodeMetadata::initInstanceLayout.

• remove /MOD/ and /OUT/ param annotations

• make Module::moduleMeta_ be const SharedModuleMetadata

• remove a pointless WASM_CHECK_CACHEABLE_POD

• remove duplicate constructors for CodeMetadata

• remove commented out accessor methods

• serialization: make CodeCodeMetadata and CodeModuleMetadata process fields
  in the same order as their declarations, and add comments for fields that
  are not serialized.

• serialization: move CodeModuleMetadata before CodeCodeMetadata to be
  consistent with sequencing in WasmMetadata.h.

Comment 27

[Julian Seward [:jseward]]

Attachment: Bug 1891182 - part 15 - move ModuleElemSegmentVector from CodeMetadata to ModuleMetadata. r=rhunt.

This patch moves ModuleElemSegmentVector from CodeMetadata to ModuleMetadata.
A vector of RefTypes is added to CodeMetadata, and this is a copy of the
.elemType fields in the ModuleElemSegmentVector.

When a wasm::Module is finally created, the ModuleElemSegmentVector is
std::move'd into the Module -- this is functionality that already exists.
Prior to this patch, the vector was moved from the CodeMetadata; now it is
moved from the ModuleMetadata. It is intended that a future patch will remove
the duplicated field in Module, hence the move, and so the
ModuleElemSegmentVector will be used directly from its new home in
ModuleMetadata.

Comment 28

[Julian Seward [:jseward]]

FTR .. I just pushed an updated stack of patches, including two new ones
(part 14, part 15) to Phabricator. The base revision is now 741936:2baa05b3de23.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14d7c72951a1
part 01 - rename ModuleEnvironment to ModuleMetadata.  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/da65995c741f
part 02 - tidy up WasmMacroAssembler::WasmMacroAssembler.  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/c5d7634e798c
part 05 - rename DataSegmentEnv/CustomSectionEnv from *Env to *Range.  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/27b0da0ed147
part 06 - move instance layout code to ModuleMetadata::doInstanceLayout.  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/57d7a29c7bf3
part 07 - create CodeMetadata by splitting ModuleMetadata.  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/3f85dccc2393
part 08 - Move ModuleMetadata and CodeMetadata to new file WasmMetadata.h.  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/0b2cb81dc31a
part 09 - Move fields out of Metadata into CodeMetadata.  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/819d998f9604
part 10 - remove Metadata and make AsmJSMetadata standalone.  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/cafacc660e35
part 11 - rename what remains of Metadata to CodeMetadataForAsmJS.  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/b8119f78ea0d
part 12 - remove Module::{imports,exports} because they are duplicated in ModuleMetadata.  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/a361f0887fe3
part 13 - remove ModuleMetadata from ModuleGenerator.  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/a50ad1347815
part 14 - address previous review comments.  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/ea9c94950374
part 15 - move ModuleElemSegmentVector from CodeMetadata to ModuleMetadata.  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/1189b410dfe8
apply code formatting via Lando

Comment 30

[Cristian Tuns]

Backed out for causing failures in awsy/test_memory_usage.py

• Backout link
• Push with failures
• Failure Log
• Failure line: PROCESS-CRASH | application crashed [@ js::wasm::ShareableBase<js::CodeMetadataForAsmJS>::sizeOfIncludingThisIfNotSeen(unsigned long (*)] | awsy/test_memory_usage.py TestMemoryUsage.test_open_tabs

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6de5a1296c7
part 01 - rename ModuleEnvironment to ModuleMetadata.  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/24107132779a
part 02 - tidy up WasmMacroAssembler::WasmMacroAssembler.  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/6f81a77c7e22
part 05 - rename DataSegmentEnv/CustomSectionEnv from *Env to *Range.  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/8183aac026d8
part 06 - move instance layout code to ModuleMetadata::doInstanceLayout.  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/3e236155d66b
part 07 - create CodeMetadata by splitting ModuleMetadata.  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/b75eed8eaf59
part 08 - Move ModuleMetadata and CodeMetadata to new file WasmMetadata.h.  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/2cd5731382a2
part 09 - Move fields out of Metadata into CodeMetadata.  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/ef79fb63a43c
part 10 - remove Metadata and make AsmJSMetadata standalone.  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/817bfbebcbe7
part 11 - rename what remains of Metadata to CodeMetadataForAsmJS.  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/85c3df70aee8
part 12 - remove Module::{imports,exports} because they are duplicated in ModuleMetadata.  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/d231233e6e1b
part 13 - remove ModuleMetadata from ModuleGenerator.  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/cb5bf2009b48
part 14 - address previous review comments.  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/e624ca90c9bf
part 15 - move ModuleElemSegmentVector from CodeMetadata to ModuleMetadata.  r=rhunt.
https://hg.mozilla.org/integration/autoland/rev/8a7e6cf0a5b9
apply code formatting via Lando

Comment 32

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/f6de5a1296c7
https://hg.mozilla.org/mozilla-central/rev/24107132779a
https://hg.mozilla.org/mozilla-central/rev/6f81a77c7e22
https://hg.mozilla.org/mozilla-central/rev/8183aac026d8
https://hg.mozilla.org/mozilla-central/rev/3e236155d66b
https://hg.mozilla.org/mozilla-central/rev/b75eed8eaf59
https://hg.mozilla.org/mozilla-central/rev/2cd5731382a2
https://hg.mozilla.org/mozilla-central/rev/ef79fb63a43c
https://hg.mozilla.org/mozilla-central/rev/817bfbebcbe7
https://hg.mozilla.org/mozilla-central/rev/85c3df70aee8
https://hg.mozilla.org/mozilla-central/rev/d231233e6e1b
https://hg.mozilla.org/mozilla-central/rev/cb5bf2009b48
https://hg.mozilla.org/mozilla-central/rev/e624ca90c9bf
https://hg.mozilla.org/mozilla-central/rev/8a7e6cf0a5b9