Bug 165957 - WebAssembly: unique function signatures
Summary: WebAssembly: unique function signatures
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: JF Bastien
URL:
Keywords: InRadar
Depends on:
Blocks: 161709 165511 166037 165833
  Show dependency treegraph
 
Reported: 2016-12-16 10:20 PST by JF Bastien
Modified: 2016-12-20 10:55 PST (History)
7 users (show)

See Also:


Attachments
WIP (77.68 KB, patch)
2016-12-18 22:42 PST, JF Bastien
jfbastien: commit-queue-
Details | Formatted Diff | Diff
WIP (80.46 KB, patch)
2016-12-19 10:35 PST, JF Bastien
jfbastien: commit-queue-
Details | Formatted Diff | Diff
WIP (81.81 KB, patch)
2016-12-19 11:11 PST, JF Bastien
jfbastien: commit-queue-
Details | Formatted Diff | Diff
patch (91.01 KB, patch)
2016-12-19 14:13 PST, JF Bastien
no flags Details | Formatted Diff | Diff
patch (91.08 KB, patch)
2016-12-19 14:40 PST, JF Bastien
no flags Details | Formatted Diff | Diff
patch (91.37 KB, patch)
2016-12-19 15:24 PST, JF Bastien
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (1011.57 KB, application/zip)
2016-12-19 16:44 PST, Build Bot
no flags Details
patch (91.59 KB, patch)
2016-12-19 19:09 PST, JF Bastien
sbarati: review+
sbarati: commit-queue-
Details | Formatted Diff | Diff
patch (91.87 KB, patch)
2016-12-20 10:11 PST, JF Bastien
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description JF Bastien 2016-12-16 10:20:47 PST
Put them on the global WebAssembly object, in a hash table of Signature -> SignatureIndex (the latter being an integer index into a Vector of Signature*). This is required for call_indirect to work, and for wasm<->wasm call signature checks to work. The signatures will "leak" because Modules aren't ref-counted, but we're going to enforce an upper limit (bug #165833) on their number so realistically there won't be that many signatures total.
Comment 1 JF Bastien 2016-12-18 22:42:16 PST
Created attachment 297453 [details]
WIP

Most code is touched, existing tests are sad, needs more tests of uniqueness, needs rebasing.
Comment 2 Radar WebKit Bug Importer 2016-12-19 09:29:21 PST
<rdar://problem/29735737>
Comment 3 JF Bastien 2016-12-19 10:35:11 PST
Created attachment 297465 [details]
WIP

Add a test, but something is wrong with it.

I still need to rebase too.
Comment 4 JF Bastien 2016-12-19 11:11:31 PST
Created attachment 297468 [details]
WIP

Rebased.
Comment 5 JF Bastien 2016-12-19 14:13:13 PST
Created attachment 297477 [details]
patch

I believe that this is ready to land.
Comment 6 JF Bastien 2016-12-19 14:19:13 PST
I'll need a lock for SignatureInformation since https://bugs.webkit.org/show_bug.cgi?id=165993 threads it.
Comment 7 JF Bastien 2016-12-19 14:40:08 PST
Created attachment 297479 [details]
patch

Lock when adding an looking up values to avoid bug #297478 from making this racy.
Comment 8 Saam Barati 2016-12-19 14:50:34 PST
Comment on attachment 297477 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297477&action=review

Patch mostly LGTM, just some questions/concerns.

> Source/JavaScriptCore/ChangeLog:11
> +        Signatures in a Module's Type section can be duplicated, we
> +        therefore need to unique them so that call_indirect works as
> +        expected. This patch makes that narrow usecase function correctly.

Nit: You should say why call_indirect would work as expected here. There are ways to make it work without uniquing signatures, but if we unique signatures, we can use a single integer compare.

> Source/JavaScriptCore/wasm/WasmFormat.h:193
> +    Vector<SignatureIndex> importFunctionSignatureIndices;
> +    Vector<SignatureIndex> internalFunctionSignatureIndices;

Style: FWIW, I like the old names better.

> Source/JavaScriptCore/wasm/WasmFormat.h:237
> +    // FIXME pack the SignatureIndex and the code pointer into one 64-bit value. https://bugs.webkit.org/show_bug.cgi?id=165511

Do we actually care about doing this? I think it'd make call_indirect slower. (Maybe not noticeably slower, though?)

> Source/JavaScriptCore/wasm/WasmModuleParser.cpp:602
> +        m_result.module->data.uncheckedAppend(Segment::createPtr(segment));

Why do we heap allocate the actual Segment? Seems weird that this isn't just a  Vector<Segment>.
Am I missing something as to why this is needed?

> Source/JavaScriptCore/wasm/WasmSignature.cpp:105
> +        Signature* invalidSignature = Signature::createInvalid();
> +        auto addResult = info->m_signatureMap.add(SignatureHash { invalidSignature }, Signature::invalidIndex);
> +        ASSERT(addResult);
> +        ASSERT(Signature::invalidIndex == addResult.iterator->value);
> +        info->m_signatures.append(invalidSignature);

This should happen inside SignatureInformation() constructor.

> Source/JavaScriptCore/wasm/WasmSignature.cpp:111
> +    if (addResult) {

What is this branch checking for? Can you use something that's more obvious than the operator bool?

> Source/JavaScriptCore/wasm/WasmSignature.h:53
> +    static const constexpr SignatureArgCount m_retCount = 1;

Style: This shouldn't be prefixed by "m"_ it should be prefixed by "s_"

> Source/JavaScriptCore/wasm/WasmSignature.h:54
> +    typedef uint64_t allocationSizeRoundsUpTo;

Why?

> Source/JavaScriptCore/wasm/WasmSignature.h:105
> +struct SignatureHash {

This class confuses me. Why not just hash Signature directly?

> Source/JavaScriptCore/wasm/WasmSignature.h:158
> +    static SignatureIndex WARN_UNUSED_RETURN adopt(VM*, Signature*);

This function confuses me. Why not just add the Signature by value? What are the semantics once this is called? Is it now the SignatureInformation's job to destroy the Signature*? If so, how does it know how it was created? Is it guaranteed to by by the create function? It feels weird that if this doesn't add Signature* the hashmap, it must destroy it immediately.
Comment 9 Saam Barati 2016-12-19 14:53:21 PST
Comment on attachment 297479 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297479&action=review

> Source/JavaScriptCore/wasm/WasmSignature.cpp:60
> +{
> +    // Assumes over-allocated memory was zero-initialized.
> +    unsigned accumulator = 0xa1bcedd8u;
> +    const auto* pos = reinterpret_cast<const allocationSizeRoundsUpTo*>(this);
> +    for (uint32_t i = 0; i != allocatedSize(argumentCount()) / sizeof(allocationSizeRoundsUpTo); ++i)
> +        accumulator = WTF::pairIntHash(accumulator, WTF::IntHash<allocationSizeRoundsUpTo>::hash(*pos));
> +    return accumulator;
> +}

This loop feels super weird to me.
Why not just do the obvious thing over Types for return type and argument types and argument count?

> Source/JavaScriptCore/wasm/WasmSignature.cpp:64
> +    // Hashing relies on allocation zero-initializing trailing elements.

Why?
Comment 10 JF Bastien 2016-12-19 15:24:44 PST
Created attachment 297484 [details]
patch

Address comments, but there are still open questions. I may be holding WTF wrong :-)


> > Source/JavaScriptCore/wasm/WasmFormat.h:193
> > +    Vector<SignatureIndex> importFunctionSignatureIndices;
> > +    Vector<SignatureIndex> internalFunctionSignatureIndices;
> 
> Style: FWIW, I like the old names better.

Sure, but now we have this extra indirection. I initially didn't rename some things (using "signature" as a stand-in for "signature index" when it was pretty obvious) but the inconsistency was confusing. It got extra confusing when both were side-by-side, so I think this is better.


> > Source/JavaScriptCore/wasm/WasmFormat.h:237
> > +    // FIXME pack the SignatureIndex and the code pointer into one 64-bit value. https://bugs.webkit.org/show_bug.cgi?id=165511
> 
> Do we actually care about doing this? I think it'd make call_indirect
> slower. (Maybe not noticeably slower, though?)

Maybe? I'll do it separately, and of course benchmark it. I think it could help.


> > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:602
> > +        m_result.module->data.uncheckedAppend(Segment::createPtr(segment));
> 
> Why do we heap allocate the actual Segment? Seems weird that this isn't just
> a  Vector<Segment>.
> Am I missing something as to why this is needed?

Yes, it's heap allocated.


> > Source/JavaScriptCore/wasm/WasmSignature.cpp:105
> > +        Signature* invalidSignature = Signature::createInvalid();
> > +        auto addResult = info->m_signatureMap.add(SignatureHash { invalidSignature }, Signature::invalidIndex);
> > +        ASSERT(addResult);
> > +        ASSERT(Signature::invalidIndex == addResult.iterator->value);
> > +        info->m_signatures.append(invalidSignature);
> 
> This should happen inside SignatureInformation() constructor.

I just changed it to the init_once thing, I think it's more natural in a single lambda to bound the scope.


> > Source/JavaScriptCore/wasm/WasmSignature.cpp:111
> > +    if (addResult) {
> 
> What is this branch checking for? Can you use something that's more obvious
> than the operator bool?

That seems to be what other uses of hash table do. The API is:

    template<typename IteratorType> struct HashTableAddResult {
        HashTableAddResult() : isNewEntry(false) { }
        HashTableAddResult(IteratorType iter, bool isNewEntry) : iterator(iter), isNewEntry(isNewEntry) { }
        IteratorType iterator;
        bool isNewEntry;

        explicit operator bool() const { return isNewEntry; }
    };

Granted, I didn't find the other parts of custom HashMap intuitive :-)


> > Source/JavaScriptCore/wasm/WasmSignature.h:54
> > +    typedef uint64_t allocationSizeRoundsUpTo;
> 
> Why?

So that we can hash more than a byte at a time (which is the size of Type).


> > Source/JavaScriptCore/wasm/WasmSignature.h:105
> > +struct SignatureHash {
> 
> This class confuses me. Why not just hash Signature directly?

What do you mean? The HashMap doesn't hold a Signature as key, only a reference to one. I'm not sure I get what you're suggesting: HashMap<Signature, SignatureIndex> doesn't work, right?


> > Source/JavaScriptCore/wasm/WasmSignature.h:158
> > +    static SignatureIndex WARN_UNUSED_RETURN adopt(VM*, Signature*);
> 
> This function confuses me. Why not just add the Signature by value? What are
> the semantics once this is called? Is it now the SignatureInformation's job
> to destroy the Signature*? If so, how does it know how it was created? Is it
> guaranteed to by by the create function? It feels weird that if this doesn't
> add Signature* the hashmap, it must destroy it immediately.

We need a Signature to hash one, so the parser *must* use the create method (there's no other way to create a signature). When it already exists, adopt destroys it, otherwise it keeps it. Either way it's adopted it.

In exchange, the parser gets a SignatureIndex. It gave the Signature away for adoption and doesn't own it anymore.

I can rejigger this, but I'm not sure what you're suggesting instead.


(In reply to comment #9)
> Comment on attachment 297479 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297479&action=review
> 
> > Source/JavaScriptCore/wasm/WasmSignature.cpp:60
> > +{
> > +    // Assumes over-allocated memory was zero-initialized.
> > +    unsigned accumulator = 0xa1bcedd8u;
> > +    const auto* pos = reinterpret_cast<const allocationSizeRoundsUpTo*>(this);
> > +    for (uint32_t i = 0; i != allocatedSize(argumentCount()) / sizeof(allocationSizeRoundsUpTo); ++i)
> > +        accumulator = WTF::pairIntHash(accumulator, WTF::IntHash<allocationSizeRoundsUpTo>::hash(*pos));
> > +    return accumulator;
> > +}
> 
> This loop feels super weird to me.
> Why not just do the obvious thing over Types for return type and argument
> types and argument count?

What's the obvious thing? I looked around and this seemed to be the best and fastest solution (SHA-1 would be slow). Is there something else?


> > Source/JavaScriptCore/wasm/WasmSignature.cpp:64
> > +    // Hashing relies on allocation zero-initializing trailing elements.
> 
> Why?

Above, if we over-allocate then we can hash 64-bit at a time.
Comment 11 Saam Barati 2016-12-19 16:32:34 PST
(In reply to comment #10)
> Created attachment 297484 [details]
> patch
> 
> Address comments, but there are still open questions. I may be holding WTF
> wrong :-)
> 
> 
> > > Source/JavaScriptCore/wasm/WasmFormat.h:193
> > > +    Vector<SignatureIndex> importFunctionSignatureIndices;
> > > +    Vector<SignatureIndex> internalFunctionSignatureIndices;
> > 
> > Style: FWIW, I like the old names better.
> 
> Sure, but now we have this extra indirection. I initially didn't rename some
> things (using "signature" as a stand-in for "signature index" when it was
> pretty obvious) but the inconsistency was confusing. It got extra confusing
> when both were side-by-side, so I think this is better.
> 
> 
> > > Source/JavaScriptCore/wasm/WasmFormat.h:237
> > > +    // FIXME pack the SignatureIndex and the code pointer into one 64-bit value. https://bugs.webkit.org/show_bug.cgi?id=165511
> > 
> > Do we actually care about doing this? I think it'd make call_indirect
> > slower. (Maybe not noticeably slower, though?)
> 
> Maybe? I'll do it separately, and of course benchmark it. I think it could
> help.
> 
> 
> > > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:602
> > > +        m_result.module->data.uncheckedAppend(Segment::createPtr(segment));
> > 
> > Why do we heap allocate the actual Segment? Seems weird that this isn't just
> > a  Vector<Segment>.
> > Am I missing something as to why this is needed?
> 
> Yes, it's heap allocated.
Ignore me. I forgot that Segment is a variable sized allocation.

> 
> 
> > > Source/JavaScriptCore/wasm/WasmSignature.cpp:105
> > > +        Signature* invalidSignature = Signature::createInvalid();
> > > +        auto addResult = info->m_signatureMap.add(SignatureHash { invalidSignature }, Signature::invalidIndex);
> > > +        ASSERT(addResult);
> > > +        ASSERT(Signature::invalidIndex == addResult.iterator->value);
> > > +        info->m_signatures.append(invalidSignature);
> > 
> > This should happen inside SignatureInformation() constructor.
> 
> I just changed it to the init_once thing, I think it's more natural in a
> single lambda to bound the scope.
You can have multiple VMs in a process.

Regardless of that, this is clearly work that should be in the constructor.
I think it's bad style to require every user of a class to do the exact same
initialization work. If someone else were to use this class besides VM, they'd
need to duplicate work.

> 
> 
> > > Source/JavaScriptCore/wasm/WasmSignature.cpp:111
> > > +    if (addResult) {
> > 
> > What is this branch checking for? Can you use something that's more obvious
> > than the operator bool?
> 
> That seems to be what other uses of hash table do. The API is:
Why not just .isNewEntry directly? That reads clearer to me.

> 
>     template<typename IteratorType> struct HashTableAddResult {
>         HashTableAddResult() : isNewEntry(false) { }
>         HashTableAddResult(IteratorType iter, bool isNewEntry) :
> iterator(iter), isNewEntry(isNewEntry) { }
>         IteratorType iterator;
>         bool isNewEntry;
> 
>         explicit operator bool() const { return isNewEntry; }
>     };
> 
> Granted, I didn't find the other parts of custom HashMap intuitive :-)
> 
> 
> > > Source/JavaScriptCore/wasm/WasmSignature.h:54
> > > +    typedef uint64_t allocationSizeRoundsUpTo;
> > 
> > Why?
> 
> So that we can hash more than a byte at a time (which is the size of Type).
IMO, this makes the code barely more unoptimized for something that doesn't
need to be performant at the cost of making the code less readable.
I'm OK with it, but not in love with it.

> 
> 
> > > Source/JavaScriptCore/wasm/WasmSignature.h:105
> > > +struct SignatureHash {
> > 
> > This class confuses me. Why not just hash Signature directly?
> 
> What do you mean? The HashMap doesn't hold a Signature as key, only a
> reference to one. I'm not sure I get what you're suggesting:
> HashMap<Signature, SignatureIndex> doesn't work, right?
Why doesn't this work?

> 
> 
> > > Source/JavaScriptCore/wasm/WasmSignature.h:158
> > > +    static SignatureIndex WARN_UNUSED_RETURN adopt(VM*, Signature*);
> > 
> > This function confuses me. Why not just add the Signature by value? What are
> > the semantics once this is called? Is it now the SignatureInformation's job
> > to destroy the Signature*? If so, how does it know how it was created? Is it
> > guaranteed to by by the create function? It feels weird that if this doesn't
> > add Signature* the hashmap, it must destroy it immediately.
> 
> We need a Signature to hash one, so the parser *must* use the create method
> (there's no other way to create a signature). When it already exists, adopt
> destroys it, otherwise it keeps it. Either way it's adopted it.
> 
> In exchange, the parser gets a SignatureIndex. It gave the Signature away
> for adoption and doesn't own it anymore.
> 
> I can rejigger this, but I'm not sure what you're suggesting instead.
> 
> 
> (In reply to comment #9)
> > Comment on attachment 297479 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=297479&action=review
> > 
> > > Source/JavaScriptCore/wasm/WasmSignature.cpp:60
> > > +{
> > > +    // Assumes over-allocated memory was zero-initialized.
> > > +    unsigned accumulator = 0xa1bcedd8u;
> > > +    const auto* pos = reinterpret_cast<const allocationSizeRoundsUpTo*>(this);
> > > +    for (uint32_t i = 0; i != allocatedSize(argumentCount()) / sizeof(allocationSizeRoundsUpTo); ++i)
> > > +        accumulator = WTF::pairIntHash(accumulator, WTF::IntHash<allocationSizeRoundsUpTo>::hash(*pos));
> > > +    return accumulator;
> > > +}
> > 
> > This loop feels super weird to me.
> > Why not just do the obvious thing over Types for return type and argument
> > types and argument count?
> 
> What's the obvious thing? I looked around and this seemed to be the best and
> fastest solution (SHA-1 would be slow). Is there something else?
I think it's just weird that we're rounding up just to be able to increment through the blob 8 bytes at a time.

> 
> 
> > > Source/JavaScriptCore/wasm/WasmSignature.cpp:64
> > > +    // Hashing relies on allocation zero-initializing trailing elements.
> > 
> > Why?
> 
> Above, if we over-allocate then we can hash 64-bit at a time.
I guess I just don't see why we need this "optimization" at the complexity cost. But that's my 2c.
Comment 12 Saam Barati 2016-12-19 16:38:19 PST
(In reply to comment #11)
> (In reply to comment #10)
> > Created attachment 297484 [details]
> > patch
> > 
> > Address comments, but there are still open questions. I may be holding WTF
> > wrong :-)
> > 
> > 
> > > > Source/JavaScriptCore/wasm/WasmFormat.h:193
> > > > +    Vector<SignatureIndex> importFunctionSignatureIndices;
> > > > +    Vector<SignatureIndex> internalFunctionSignatureIndices;
> > > 
> > > Style: FWIW, I like the old names better.
> > 
> > Sure, but now we have this extra indirection. I initially didn't rename some
> > things (using "signature" as a stand-in for "signature index" when it was
> > pretty obvious) but the inconsistency was confusing. It got extra confusing
> > when both were side-by-side, so I think this is better.
> > 
> > 
> > > > Source/JavaScriptCore/wasm/WasmFormat.h:237
> > > > +    // FIXME pack the SignatureIndex and the code pointer into one 64-bit value. https://bugs.webkit.org/show_bug.cgi?id=165511
> > > 
> > > Do we actually care about doing this? I think it'd make call_indirect
> > > slower. (Maybe not noticeably slower, though?)
> > 
> > Maybe? I'll do it separately, and of course benchmark it. I think it could
> > help.
> > 
> > 
> > > > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:602
> > > > +        m_result.module->data.uncheckedAppend(Segment::createPtr(segment));
> > > 
> > > Why do we heap allocate the actual Segment? Seems weird that this isn't just
> > > a  Vector<Segment>.
> > > Am I missing something as to why this is needed?
> > 
> > Yes, it's heap allocated.
> Ignore me. I forgot that Segment is a variable sized allocation.
> 
> > 
> > 
> > > > Source/JavaScriptCore/wasm/WasmSignature.cpp:105
> > > > +        Signature* invalidSignature = Signature::createInvalid();
> > > > +        auto addResult = info->m_signatureMap.add(SignatureHash { invalidSignature }, Signature::invalidIndex);
> > > > +        ASSERT(addResult);
> > > > +        ASSERT(Signature::invalidIndex == addResult.iterator->value);
> > > > +        info->m_signatures.append(invalidSignature);
> > > 
> > > This should happen inside SignatureInformation() constructor.
> > 
> > I just changed it to the init_once thing, I think it's more natural in a
> > single lambda to bound the scope.
> You can have multiple VMs in a process.
> 
> Regardless of that, this is clearly work that should be in the constructor.
> I think it's bad style to require every user of a class to do the exact same
> initialization work. If someone else were to use this class besides VM,
> they'd
> need to duplicate work.
> 
> > 
> > 
> > > > Source/JavaScriptCore/wasm/WasmSignature.cpp:111
> > > > +    if (addResult) {
> > > 
> > > What is this branch checking for? Can you use something that's more obvious
> > > than the operator bool?
> > 
> > That seems to be what other uses of hash table do. The API is:
> Why not just .isNewEntry directly? That reads clearer to me.
> 
> > 
> >     template<typename IteratorType> struct HashTableAddResult {
> >         HashTableAddResult() : isNewEntry(false) { }
> >         HashTableAddResult(IteratorType iter, bool isNewEntry) :
> > iterator(iter), isNewEntry(isNewEntry) { }
> >         IteratorType iterator;
> >         bool isNewEntry;
> > 
> >         explicit operator bool() const { return isNewEntry; }
> >     };
> > 
> > Granted, I didn't find the other parts of custom HashMap intuitive :-)
> > 
> > 
> > > > Source/JavaScriptCore/wasm/WasmSignature.h:54
> > > > +    typedef uint64_t allocationSizeRoundsUpTo;
> > > 
> > > Why?
> > 
> > So that we can hash more than a byte at a time (which is the size of Type).
> IMO, this makes the code barely more unoptimized for something that doesn't
> need to be performant at the cost of making the code less readable.
> I'm OK with it, but not in love with it.
> 
> > 
> > 
> > > > Source/JavaScriptCore/wasm/WasmSignature.h:105
> > > > +struct SignatureHash {
> > > 
> > > This class confuses me. Why not just hash Signature directly?
> > 
> > What do you mean? The HashMap doesn't hold a Signature as key, only a
> > reference to one. I'm not sure I get what you're suggesting:
> > HashMap<Signature, SignatureIndex> doesn't work, right?
> Why doesn't this work?
Oh I see why, it's because Signature is variable sized.
> 
> > 
> > 
> > > > Source/JavaScriptCore/wasm/WasmSignature.h:158
> > > > +    static SignatureIndex WARN_UNUSED_RETURN adopt(VM*, Signature*);
> > > 
> > > This function confuses me. Why not just add the Signature by value? What are
> > > the semantics once this is called? Is it now the SignatureInformation's job
> > > to destroy the Signature*? If so, how does it know how it was created? Is it
> > > guaranteed to by by the create function? It feels weird that if this doesn't
> > > add Signature* the hashmap, it must destroy it immediately.
> > 
> > We need a Signature to hash one, so the parser *must* use the create method
> > (there's no other way to create a signature). When it already exists, adopt
> > destroys it, otherwise it keeps it. Either way it's adopted it.
> > 
> > In exchange, the parser gets a SignatureIndex. It gave the Signature away
> > for adoption and doesn't own it anymore.
> > 
> > I can rejigger this, but I'm not sure what you're suggesting instead.
> > 
> > 
> > (In reply to comment #9)
> > > Comment on attachment 297479 [details]
> > > patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=297479&action=review
> > > 
> > > > Source/JavaScriptCore/wasm/WasmSignature.cpp:60
> > > > +{
> > > > +    // Assumes over-allocated memory was zero-initialized.
> > > > +    unsigned accumulator = 0xa1bcedd8u;
> > > > +    const auto* pos = reinterpret_cast<const allocationSizeRoundsUpTo*>(this);
> > > > +    for (uint32_t i = 0; i != allocatedSize(argumentCount()) / sizeof(allocationSizeRoundsUpTo); ++i)
> > > > +        accumulator = WTF::pairIntHash(accumulator, WTF::IntHash<allocationSizeRoundsUpTo>::hash(*pos));
> > > > +    return accumulator;
> > > > +}
> > > 
> > > This loop feels super weird to me.
> > > Why not just do the obvious thing over Types for return type and argument
> > > types and argument count?
> > 
> > What's the obvious thing? I looked around and this seemed to be the best and
> > fastest solution (SHA-1 would be slow). Is there something else?
> I think it's just weird that we're rounding up just to be able to increment
> through the blob 8 bytes at a time.
> 
> > 
> > 
> > > > Source/JavaScriptCore/wasm/WasmSignature.cpp:64
> > > > +    // Hashing relies on allocation zero-initializing trailing elements.
> > > 
> > > Why?
> > 
> > Above, if we over-allocate then we can hash 64-bit at a time.
> I guess I just don't see why we need this "optimization" at the complexity
> cost. But that's my 2c.
Comment 13 Build Bot 2016-12-19 16:44:13 PST
Comment on attachment 297484 [details]
patch

Attachment 297484 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2756246

New failing tests:
imported/w3c/web-platform-tests/IndexedDB/idbcursor-direction-index-keyrange.htm
Comment 14 Build Bot 2016-12-19 16:44:16 PST
Created attachment 297494 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 15 JF Bastien 2016-12-19 19:09:40 PST
Created attachment 297499 [details]
patch

Address comments.
Comment 16 Saam Barati 2016-12-19 23:00:51 PST
Comment on attachment 297499 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297499&action=review

r=me if you address the call_once bug

> Source/JavaScriptCore/wasm/WasmFormat.h:151
> +    static void destroy(Segment *);

Style: No space before *

> Source/JavaScriptCore/wasm/WasmFormat.h:153
> +    static Ptr createPtr(Segment*);

Maybe it's worth calling this adoptPtr since we're creating a wrapper for something we're expected to destroy

> Source/JavaScriptCore/wasm/WasmSignature.cpp:57
> +    for (uint32_t i = 0; i != allocatedSize(argumentCount()) / sizeof(allocationSizeRoundsUpTo); ++i)

Style: use < not !=

> Source/JavaScriptCore/wasm/WasmSignature.cpp:76
> +    if (!signature)

You could use RELEASE_ASSERT here

> Source/JavaScriptCore/wasm/WasmSignature.cpp:89
> +    for (size_t i = 0; i != m_signatures.size(); ++i)

Use < not !=

> Source/JavaScriptCore/wasm/WasmSignature.cpp:95
> +    static std::once_flag onceFlag;

As I said in my previous comment, this code is wrong. This needs to happen once per VM, not once per process. There can be more than one VM in a process.

The rest of this patch LGTM but this code is the only thing I see that's wrong.

> Source/JavaScriptCore/wasm/WasmSignature.cpp:97
> +        vm->m_wasmSignatureInformation = std::unique_ptr<SignatureInformation>(new SignatureInformation());

I know you said that this can't go in the constructor but I see no reason why not. Can you elaborate on why? Do you just prefer the style of this?
Comment 17 JF Bastien 2016-12-20 10:11:06 PST
Created attachment 297530 [details]
patch

Address comments.


> > Source/JavaScriptCore/wasm/WasmSignature.cpp:57
> > +    for (uint32_t i = 0; i != allocatedSize(argumentCount()) / sizeof(allocationSizeRoundsUpTo); ++i)
> 
> Style: use < not !=
> 
> > Source/JavaScriptCore/wasm/WasmSignature.cpp:89
> > +    for (size_t i = 0; i != m_signatures.size(); ++i)
> 
> Use < not !=

I keep getting this style wrong! C++ reflex :-)

> > Source/JavaScriptCore/wasm/WasmSignature.cpp:95
> > +    static std::once_flag onceFlag;
> 
> As I said in my previous comment, this code is wrong. This needs to happen
> once per VM, not once per process. There can be more than one VM in a
> process.

Ah sorry, I misunderstood what was wrong last time. You're totally right.


> > Source/JavaScriptCore/wasm/WasmSignature.cpp:97
> > +        vm->m_wasmSignatureInformation = std::unique_ptr<SignatureInformation>(new SignatureInformation());
> 
> I know you said that this can't go in the constructor but I see no reason
> why not. Can you elaborate on why? Do you just prefer the style of this?

Same here, I misunderstood. You're right, and the code is cleaner now.
Comment 18 WebKit Commit Bot 2016-12-20 10:55:15 PST
Comment on attachment 297530 [details]
patch

Clearing flags on attachment: 297530

Committed r210026: <http://trac.webkit.org/changeset/210026>
Comment 19 WebKit Commit Bot 2016-12-20 10:55:21 PST
All reviewed patches have been landed.  Closing bug.