Baldr: remove GlobalSegment and its unnecessary indirection

RESOLVED FIXED in Firefox 60

Status

()

defect
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(2 attachments)

This patch should remove one load from the hot JIT->wasm path in bug 1319203.
Attachment #8945214 - Flags: review?(bbouvier)
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`
Attachment #8945214 - Flags: review?(bbouvier) → review+
(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.
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0e8dd7f8bcf
Baldr: remove GlobalSegment (r=bbouvier)
https://hg.mozilla.org/mozilla-central/rev/d0e8dd7f8bcf
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Oops, I just noticed I should be freeing tlsData->allocatedBase, not tlsData because, in general, they can be different (bug 1374218).
Attachment #8947958 - Flags: review?(bbouvier)
Comment on attachment 8947958 [details] [diff] [review]
fix-allocated-base

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

Duh yes!
Attachment #8947958 - Flags: review?(bbouvier) → review+
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/23f8e2838e0a
Fix regression; free allocatedBase (r=bbouvier)
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?
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.
Assignee: nobody → luke
You need to log in before you can comment on or make changes to this bug.