1432956 - Baldr: remove GlobalSegment and its unnecessary indirection

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: rm-global-segment

This patch should remove one load from the hot JIT->wasm path in bug 1319203.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8945214 [details] [diff] [review]
rm-global-segment

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

Sweet, thanks!

::: js/src/wasm/WasmModule.cpp
@@ +1163,5 @@
>      SharedTableVector tables;
>      if (!instantiateTable(cx, &table, &tables))
>          return false;
>  
> +    auto tlsData = CreateTlsData(metadata().globalDataLength);

nit: s/auto/UniqueTlsData

::: js/src/wasm/WasmTypes.cpp
@@ +897,5 @@
> +    void* allocatedBase = js_calloc(TlsDataAlign + offsetof(TlsData, globalArea) + globalDataLength);
> +    if (!allocatedBase)
> +        return nullptr;
> +
> +    auto* tlsData = reinterpret_cast<TlsData*>(AlignBytes(size_t(allocatedBase), TlsDataAlign));

preexisting, Would it be more precise to use intptr_t/uintptr_t here instead of size_t?

@@ +902,5 @@
> +    tlsData->allocatedBase = allocatedBase;
> +
> +    return UniqueTlsData(tlsData);
> +}
> +

nit: blank line

::: js/src/wasm/WasmTypes.h
@@ +1529,5 @@
> +{
> +    void operator()(TlsData* tlsData) { js_free(tlsData); }
> +};
> +
> +typedef UniquePtr<TlsData, TlsDataDeleter> UniqueTlsData;

nit: let's be modern and use `using`

Comment 2

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #1)
> > +typedef UniquePtr<TlsData, TlsDataDeleter> UniqueTlsData;
> 
> nit: let's be modern and use `using`

The file uses typedefs throughout, especially for UniquePtr<T> and Vector<T>, and for plain cases like this, there's little benefit to switching the order, so I'd like to keep things consistent and instead do bulk conversions in a patch.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0e8dd7f8bcf
Baldr: remove GlobalSegment (r=bbouvier)

Comment 4

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/d0e8dd7f8bcf

Comment 5

[Luke Wagner [:luke]]

Attachment: fix-allocated-base

Oops, I just noticed I should be freeing tlsData->allocatedBase, not tlsData because, in general, they can be different (bug 1374218).

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8947958 [details] [diff] [review]
fix-allocated-base

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

Duh yes!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/23f8e2838e0a
Fix regression; free allocatedBase (r=bbouvier)

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

That being said, I wonder why no tests caught this issue: is it that the ASAN builds aren't tier 1 yet? Or is it that the js_free function would automatically align bytes of the address to free on a certain boundary that happened to be the same that ours?

Comment 9

[Luke Wagner [:luke]]

So if you malloc(N) bytes, malloc will N-byte align the returned pointer up to some upper-bound K.  On most systems, K >= 16 which is the most alignment we need.  It was only in bug 1374218 that jseward reported a configuration with less alignment.  Thus, for all the systems we regularly build on, tlsData == tlsData->allocatedBase.

Comment 10

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/23f8e2838e0a