Bug 234116 - [WebAssembly][Modules] Unify memory import handling code in both module loader and JS cases
Summary: [WebAssembly][Modules] Unify memory import handling code in both module loade...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebAssembly (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-09 17:53 PST by Asumu Takikawa
Modified: 2022-01-11 15:49 PST (History)
9 users (show)

See Also:


Attachments
Patch (23.21 KB, patch)
2021-12-09 18:50 PST, Asumu Takikawa
no flags Details | Formatted Diff | Diff
Patch (28.21 KB, patch)
2021-12-15 13:02 PST, Asumu Takikawa
no flags Details | Formatted Diff | Diff
Patch (28.32 KB, patch)
2021-12-15 22:39 PST, Asumu Takikawa
no flags Details | Formatted Diff | Diff
Patch (28.33 KB, patch)
2021-12-20 11:05 PST, Asumu Takikawa
no flags Details | Formatted Diff | Diff
Patch (26.76 KB, patch)
2022-01-07 14:32 PST, Asumu Takikawa
no flags Details | Formatted Diff | Diff
Patch (26.81 KB, patch)
2022-01-11 12:46 PST, Asumu Takikawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Asumu Takikawa 2021-12-09 17:53:18 PST
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.
Comment 1 Asumu Takikawa 2021-12-09 18:50:11 PST
Created attachment 446652 [details]
Patch
Comment 2 Asumu Takikawa 2021-12-15 13:02:19 PST
Created attachment 447280 [details]
Patch
Comment 3 Asumu Takikawa 2021-12-15 20:51:26 PST
I'm rebasing this now for the CodeBlock to CalleeGroup change and will upload a new patch soon.
Comment 4 Asumu Takikawa 2021-12-15 22:39:25 PST
Created attachment 447328 [details]
Patch
Comment 5 Yusuke Suzuki 2021-12-16 02:55:40 PST
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.
Comment 6 Radar WebKit Bug Importer 2021-12-16 18:30:51 PST
<rdar://problem/86608194>
Comment 7 Asumu Takikawa 2021-12-20 09:10:00 PST
> 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.
Comment 8 Asumu Takikawa 2021-12-20 11:05:40 PST
Created attachment 447613 [details]
Patch
Comment 9 Yusuke Suzuki 2021-12-20 11:44:48 PST
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?
Comment 10 Asumu Takikawa 2021-12-20 12:45:59 PST
(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).
Comment 11 Yusuke Suzuki 2021-12-21 00:16:37 PST
(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.
Comment 12 Asumu Takikawa 2022-01-07 14:32:31 PST
Created attachment 448631 [details]
Patch
Comment 13 Asumu Takikawa 2022-01-07 14:36:53 PST
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?
Comment 14 Darin Adler 2022-01-09 09:09:11 PST
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.
Comment 15 Yusuke Suzuki 2022-01-09 20:57:08 PST
Comment on attachment 448631 [details]
Patch

Looks good to me too!
Comment 16 Yusuke Suzuki 2022-01-09 20:58:08 PST
(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!
Comment 17 Asumu Takikawa 2022-01-11 12:46:33 PST
Created attachment 448871 [details]
Patch
Comment 18 Yusuke Suzuki 2022-01-11 12:47:06 PST
Comment on attachment 448871 [details]
Patch

r=me
Comment 19 Asumu Takikawa 2022-01-11 12:49:00 PST
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.
Comment 20 EWS 2022-01-11 15:49:45 PST
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].