WebAssembly: unique function signatures

[JF Bastien]

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.

[JF Bastien]

Created attachment 297453 [details]
WIP

Most code is touched, existing tests are sad, needs more tests of uniqueness, needs rebasing.

[Radar WebKit Bug Importer]

<rdar://problem/29735737>

[JF Bastien]

Created attachment 297465 [details]
WIP

Add a test, but something is wrong with it.

I still need to rebase too.

[JF Bastien]

Created attachment 297468 [details]
WIP

Rebased.

[JF Bastien]

Created attachment 297477 [details]
patch

I believe that this is ready to land.

[JF Bastien]

I'll need a lock for SignatureInformation since https://bugs.webkit.org/show_bug.cgi?id=165993 threads it.

[JF Bastien]

Created attachment 297479 [details]
patch

Lock when adding an looking up values to avoid bug #297478 from making this racy.

[Saam Barati]

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.

[Saam Barati]

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?

[JF Bastien]

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.

[Saam Barati]

(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.

[Saam Barati]

(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.

[Build Bot]

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

[Build Bot]

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

[JF Bastien]

Created attachment 297499 [details]
patch

Address comments.

[Saam Barati]

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?

[JF Bastien]

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.

[WebKit Commit Bot]

Comment on attachment 297530 [details]
patch

Clearing flags on attachment: 297530

Committed r210026: <http://trac.webkit.org/changeset/210026>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.