1753692 - Merge wasm::Instance and TlsData

Status: RESOLVED FIXED

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

Description

[Ryan Hunt [:rhunt]]

Filing this for the future.

wasm::Instance and TlsData are 1:1 and have the same lifetime. TlsData has special alignment requirements and also has a trailing global data area attached to it. Theoretically, the two could be merged by applying the alignment requirements to Instance and adding the global data area to the end of Instance.

The primary benefit would be removing 'switches' between *Instance and *TlsData when we have one and need the other.

For example, non-constant memory.copy does the following:

1. Instance call in JIT loads instance pointer from WasmTlsReg
2. Call to Instance::memCopy_mNN with instance pointer
3. Instance::memCopy_mNN loads Instance::tls_ to get the JSContext*

The secondary benefit would be simplifying the decision about where to put data, as either location may be appropriate.

The primary drawback is that this would be a very invasive change, especially if there is renaming done. The 'tls' naming is used extensively. A potential other drawback is that by combining the fields of TlsData and Instance, we may push certain fields out of range for optimal instruction load encodings in JIT code. This would need further investigation.

Comment 1

[Lars T Hansen [:lth]]

For bug 1709578 I will probably want to assume that the memoryBase is no more than 4K away from whatever base pointer we use. This base pointer will end up in a FuncRef table where the Tls is now, and I will want to assume that loading from *(tls + offsetof(TlsData, memoryBase)) will fault if the tls is null, so that I don't have to do a null check.

Comment 2

[Ryan Hunt [:rhunt]]

Attachment: Bug 1753692 - wasm: Reduce cost of WasmInstance.h for when it is transitively needed in more places. r?lth

Comment 3

[Ryan Hunt [:rhunt]]

Attachment: Bug 1753692 - wasm: Protect members of TlsData in preparation to be moved to Instance. r?lth

Depends on D139535

Comment 4

[Ryan Hunt [:rhunt]]

Attachment: Bug 1753692 - wasm: Merge TlsData into Instance. r?lth

1. Move fields and accessors from TlsData to Instance
2. Allocate Instance using extra alignment
3. Leave TlsData as a subtype of Instance with no fields
   • This leaves all references to TlsData as valid, although
     the type is never constructed
4. Leave instance->tlsData() and tls->instance() as identity
   operations to be cleaned up later.

The net result is that tlsdata and instance are the same
object in memory, just with different names.

Depends on D139536

Comment 5

[Ryan Hunt [:rhunt]]

Attachment: Bug 1753692 - wasm: Remove stub TlsData type and point references to Instance. r?lth

Depends on D139537

Comment 6

[Ryan Hunt [:rhunt]]

Attachment: Bug 1753692 - wasm: Replace 'instance->tlsData()' with just 'instance'. r?lth

Depends on D139538

Comment 7

[Ryan Hunt [:rhunt]]

Attachment: Bug 1753692 - wasm: Replace 'tlsData->instance()' with just 'tlsData'. r?lth

Depends on D139539

Comment 8

[Ryan Hunt [:rhunt]]

Attachment: Bug 1753692 - wasm: Avoid redundant load of instance from tls in JIT. r?lth

Stop loading tls->instance and just use instance, as they are
now the same thing.

Depends on D139540

Comment 9

[Ryan Hunt [:rhunt]]

I've slowly written the patches to do this. Posting my work so far that does everything except for the renaming of 'tls' to 'instance' across variables/fields.

I think the end result is fairly nice, the last three patches illustrate all of the pointer chasing that is removed by merging these two concepts. However, I think there will be a lot of churn in the patches to do the renaming so I want to be sure everyone agrees this is worth doing before writing those.

Comment 10

[Lars T Hansen [:lth]]

Soft r+ on this; a wrenching change but very plausible and a desirable simplification.

Comment 11

[Ryan Hunt [:rhunt]]

Attachment: Bug 1753692 - wasm: Re-order Instance fields and add assert for compact encodings. r?lth

This re-orders some Instance fields for clarity, and adds comments
and assertions that the first fields of Instance are reserved for
critical data that should be accessable using compact encodings.

Depends on D139541

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a430610af717
wasm: Reduce cost of WasmInstance.h for when it is transitively needed in more places. r=lth
https://hg.mozilla.org/integration/autoland/rev/a961313a61ee
wasm: Protect members of TlsData in preparation to be moved to Instance. r=lth
https://hg.mozilla.org/integration/autoland/rev/0942b789d2eb
wasm: Merge TlsData into Instance. r=lth
https://hg.mozilla.org/integration/autoland/rev/a18404abc271
wasm: Remove stub TlsData type and point references to Instance. r=lth
https://hg.mozilla.org/integration/autoland/rev/26289a2d4b03
wasm: Replace 'instance->tlsData()' with just 'instance'. r=lth
https://hg.mozilla.org/integration/autoland/rev/f4192317dd41
wasm: Replace 'tlsData->instance()' with just 'tlsData'. r=lth
https://hg.mozilla.org/integration/autoland/rev/d25caad49188
wasm: Avoid redundant load of instance from tls in JIT. r=lth
https://hg.mozilla.org/integration/autoland/rev/30534620399d
wasm: Re-order Instance fields and add assert for compact encodings. r=lth

Comment 13

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/a430610af717
https://hg.mozilla.org/mozilla-central/rev/a961313a61ee
https://hg.mozilla.org/mozilla-central/rev/0942b789d2eb
https://hg.mozilla.org/mozilla-central/rev/a18404abc271
https://hg.mozilla.org/mozilla-central/rev/26289a2d4b03
https://hg.mozilla.org/mozilla-central/rev/f4192317dd41
https://hg.mozilla.org/mozilla-central/rev/d25caad49188
https://hg.mozilla.org/mozilla-central/rev/30534620399d