Bug 234116 – [WebAssembly][Modules] Unify memory import handling code in both module loader and JS cases

• Status: RESOLVED
• Resolution: FIXED
• Component: WebAssembly

[Asumu Takikawa]

Currently most initialization of Wasm module imports are done in the `WebAssemblyModuleRecord` class, and only memory imports are handled in `JSWebAssemblyInstance`.

These code paths can be unified now that memory imports are supported when Wasm modules are used via the module loader (in https://bugs.webkit.org/show_bug.cgi?id=184745). Unifying the memory code will finish the process of moving the import initialization to the module record.

[Asumu Takikawa]

Created attachment 446652 [details]
Patch

[Asumu Takikawa]

Created attachment 447280 [details]
Patch

[Asumu Takikawa]

I'm rebasing this now for the CodeBlock to CalleeGroup change and will upload a new patch soon.

[Asumu Takikawa]

Created attachment 447328 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 447328 [details]
Patch

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

Oops, I found one issue. So commented.

> Source/JavaScriptCore/wasm/WasmModule.cpp:123
> +        RefPtr<CalleeGroup> calleeGroup;
> +        calleeGroup = m_calleeGroups[i];

Let's write it in one line. And since we never release once-created CalleeGroup, we do not need to ref here.

CalleeGroup* calleeGroup = m_calleeGroups[i].get();

> Source/JavaScriptCore/wasm/WasmModule.cpp:126
> +        if (calleeGroup && (!calleeGroup->compilationFinished() || calleeGroup->runnable()))
> +            continue;

This condition is saying, if calleeGroup exists but if it is not (!calleeGroup->compilationFinished() || calleeGroup->runnable()), then we override with the new one.
Is it right behavior? Destroying already-existing calleeGroup sounds incorrect to me.

[Radar WebKit Bug Importer]

<rdar://problem/86608194>

[Asumu Takikawa]

> This condition is saying, if calleeGroup exists but if it is not (!calleeGroup->compilationFinished() || calleeGroup->runnable()), then we override with the new one.
Is it right behavior? Destroying already-existing calleeGroup sounds incorrect to me.

I think the condition is saying something a bit different. It should be a negation of the related condition earlier in WasmModule.cpp in getOrCreateCalleeGroup:

> (!calleeGroup || (calleeGroup->compilationFinished() && !calleeGroup->runnable()))

vs

> Source/JavaScriptCore/wasm/WasmModule.cpp:126
> +        if (calleeGroup && (!calleeGroup->compilationFinished() || calleeGroup->runnable()))
> +            continue;

And it also continues when the condition is true.

So this should be saying, continue and skip the `createFromExisting` call if the calleeGroup exists already and unless the compilation is finished but it's still not runnable. Would it be clearer if the second half was written as the following?

> !(calleeGroup->compilationFinished() && !calleeGroup->runnable())

I'll upload a patch with this last refactoring in case that's better.

[Asumu Takikawa]

Created attachment 447613 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 447613 [details]
Patch

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

> Source/JavaScriptCore/wasm/WasmModule.cpp:125
> +        // We should only try to copy the group here if it hasn't already been compiled.
> +        if (calleeGroup && !(calleeGroup->compilationFinished() && !calleeGroup->runnable()))
> +            continue;

Ah, what I would like to ask is that, if calleeGroup is non null, then wasting that calleeGroup by `m_calleeGroups[i] = WTFMove(newBlock)` sounds wrong to me.
When does it happen and why do we need to replace it with a newly copied one?

[Asumu Takikawa]

(In reply to Yusuke Suzuki from comment #9)
> Ah, what I would like to ask is that, if calleeGroup is non null, then
> wasting that calleeGroup by `m_calleeGroups[i] = WTFMove(newBlock)` sounds
> wrong to me.
> When does it happen and why do we need to replace it with a newly copied one?

Ah ok, I think I understand now. Do you mean that if the calleeGroup exists but it's not runnable, we can instead try to recompile the existing one (probably later when the memory is available) rather than copy a new one in here?

That makes sense to me. I can upload a new patch that implements this later (in Jan, as I've run out of time and will be away from my computer for a while).

[Yusuke Suzuki]

(In reply to Asumu Takikawa from comment #10)
> (In reply to Yusuke Suzuki from comment #9)
> > Ah, what I would like to ask is that, if calleeGroup is non null, then
> > wasting that calleeGroup by `m_calleeGroups[i] = WTFMove(newBlock)` sounds
> > wrong to me.
> > When does it happen and why do we need to replace it with a newly copied one?
> 
> Ah ok, I think I understand now. Do you mean that if the calleeGroup exists
> but it's not runnable, we can instead try to recompile the existing one
> (probably later when the memory is available) rather than copy a new one in
> here?
> 
> That makes sense to me. I can upload a new patch that implements this later
> (in Jan, as I've run out of time and will be away from my computer for a
> while).

Yeah, I would like to know why this condition happens. And I think, if CalleeGroup is already created, then destroying it and replacing it sounds dangerous. Probably, initialize it with the other CalleeGroup's LLIntCallees would be better, but I first would like to know when this happens.

[Asumu Takikawa]

Created attachment 448631 [details]
Patch

[Asumu Takikawa]

I've rebased the patch for recent changes.

> Yeah, I would like to know why this condition happens. And I think, if CalleeGroup is already created, then destroying it and replacing it sounds dangerous. Probably, initialize it with the other CalleeGroup's LLIntCallees would be better, but I first would like to know when this happens.

I believe it might happen if the CalleeGroup came from a module that has already been used for another instance, but an OOM or similar event caused the compilation to fail there.

In the current patch, I removed the second part of the condition so that it doesn't try to replace the CalleeGroup. In the case where the callee group is present but not runnable, it should try to re-compile the code in the existing code path in WebAssemblyModuleRecord.cpp around line 430 where it checks if the callee group is runnable. Does that seem better?

[Darin Adler]

Comment on attachment 448631 [details]
Patch

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

While I’m not an expert on the Wasm subsystem, the change sounds logical, and all the tests are passing.

> Source/JavaScriptCore/wasm/WasmModule.cpp:128
> +        CalleeGroup* calleeGroup = m_calleeGroups[i].get();
> +        // We should only try to copy the group here if it hasn't already been created.
> +        // If it exists but is not runnable, it should get compiled during module evaluation.
> +        if (calleeGroup)
> +            continue;
>          Ref<CalleeGroup> newBlock = CalleeGroup::createFromExisting(static_cast<MemoryMode>(i), initialBlock);
>          m_calleeGroups[i] = WTFMove(newBlock);

The local variables are making this code less readable. It should look more like this.

    // We should only try to copy the group here if it hasn't already been created.
    // If it exists but is not runnable, it should get compiled during module evaluation.
    if (!m_calleeGroups[i])
        m_calleeGroups[i] = CalleeGroup::createFromExisting(static_cast<MemoryMode>(i), initialBlock);

In fact, I would go even further:

    // We should only try to copy the group here if it hasn't already been created.
    // If it exists but is not runnable, it should get compiled during module evaluation.
    if (auto& group = m_calleeGroups[i]; !group)
        group = CalleeGroup::createFromExisting(static_cast<MemoryMode>(i), initialBlock);

But some people might find that style subtle or confusing.

[Yusuke Suzuki]

Comment on attachment 448631 [details]
Patch

Looks good to me too!

[Yusuke Suzuki]

(In reply to Asumu Takikawa from comment #13)
> I've rebased the patch for recent changes.
> 
> > Yeah, I would like to know why this condition happens. And I think, if CalleeGroup is already created, then destroying it and replacing it sounds dangerous. Probably, initialize it with the other CalleeGroup's LLIntCallees would be better, but I first would like to know when this happens.
> 
> I believe it might happen if the CalleeGroup came from a module that has
> already been used for another instance, but an OOM or similar event caused
> the compilation to fail there.
> 
> In the current patch, I removed the second part of the condition so that it
> doesn't try to replace the CalleeGroup. In the case where the callee group
> is present but not runnable, it should try to re-compile the code in the
> existing code path in WebAssemblyModuleRecord.cpp around line 430 where it
> checks if the callee group is runnable. Does that seem better?

Sounds good to me!

[Asumu Takikawa]

Created attachment 448871 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 448871 [details]
Patch

r=me

[Asumu Takikawa]

Rebased again.

(In reply to Darin Adler from comment #14)
> The local variables are making this code less readable. It should look more like this.

Thanks! I've implemented your second suggestion in the new patch revision.

[EWS]

Committed r287903 (245938@main): <https://commits.webkit.org/245938@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 448871 [details].