Bug 177887 – WebAssembly: address no VM / JS follow-ups

• Component: WebAssembly
• Status: RESOLVED DUPLICATE

[JF Bastien]

Saam had comments in #177473 which I hadn't addressed.

[JF Bastien]

Created attachment 322703 [details]
patch

(In reply to Saam Barati from comment #30)
> Comment on attachment 322103 [details]
>
> My meta comment here is I think this patch would be a lot easier to review
> if you had split it up.

Agreed, will do so next time.


> > Source/JavaScriptCore/ChangeLog:30
> > +            purpose: one to notify of memory pressure, and the other to ask for
> 
> what do you mean by memory pressure here?

When we allocate we can get told "I found space, but it's getting tight here". There's a callback for that now, as well as one for "I didn't find any space so sorry", and "I got space, all is good".


> > Source/JavaScriptCore/ChangeLog:104
> > +            entrypoint. This triples the space allocated per instance's imported
> > +            function, but there shouldn't be that many imports. This has two upsides: it
> 
> Triples seems like a lot if there are a lot of these. I thought Emscripten
> generates a *ton* of imports?

No, it's functions that it has a lot of. If you look at Epic Zen Garden it has:

     Type start=0x0000000e end=0x000024c7 (size=0x000024b9) count: 852
   Import start=0x000024cd end=0x000055cf (size=0x00003102) count: 443
 Function start=0x000055d5 end=0x00021257 (size=0x0001bc82) count: 111860
   Global start=0x0002125d end=0x00021290 (size=0x00000033) count: 10
   Export start=0x00021296 end=0x00024fc0 (size=0x00003d2a) count: 498
     Elem start=0x00024fc6 end=0x00080dd1 (size=0x0005be0b) count: 1
     Code start=0x00080dd7 end=0x01749ab3 (size=0x016c8cdc) count: 111860
     Data start=0x01749ab9 end=0x01d3f79c (size=0x005f5ce3) count: 8153
   Custom start=0x01d3f7a2 end=0x025ae17e (size=0x0086e9dc) "name"


> > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:388
> > +            // FIXME: Because WasmToWasm call clobbers wasmContextInstance register and does not restore it, we need to restore it in the caller side.
> 
> wasmContextInstance? Isn't that redundant? Ditto elsewhere you do this.

Indeed! Right now it's just weird but context is instance, and I wanted to make that obvious. I plan to fix it as a follow-up.


> > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1206
> > +    // FIXME: when we have trap handlers, we can just let the call fail because Signature::invalidIndex is 0. https://bugs.webkit.org/show_bug.cgi?id=177210
> 
> How does this have anything to do with signature index? It seems like this
> only has to do with the code pointer we load.

I'm not sure I understand your question. This bug just says that we don't need to do the zero check if we have trap handlers, we just need to detect that we jumped to whatever is in index entry zero (another zero presumably, or 0xc0defefe) from wasm JIT code and that's enough to know that the index was invalid.


> > Source/JavaScriptCore/wasm/WasmMemory.h:99
> > +    WTF::Function<void()> m_notifyMemoryPressure;
> > +    WTF::Function<void()> m_syncTryToReclaimMemory;
> > +    WTF::Function<void(PageCount, PageCount)> m_growSuccessCallback;
> 
> Do we really want to tie these to a Memory and not a call to grow()?

The call to grow doesn't know what the embedder is, but it necessarily knows the memory. That's why I want the memory to know: so I don't have to teach the code we generate. Even better: eventually we could share a memory between two embedders, and chain these callbacks (so you grow, and then both know what the new size is and update embedder-specific info).


> > Source/JavaScriptCore/wasm/WasmThunks.cpp:94
> > +    typedef void (*Run)(JSWebAssemblyInstance*, uint32_t);
> > +    Run run = OMGPlan::runForIndex;
> 
> Is this just for documenting the type?

Yeah it forces a type check. I had a bug here where I changed the signature, and the reinterpret_cast was none the wiser :-(


> >> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:344
> >> +                [&vm, jsMemory] (Wasm::PageCount oldPageCount, Wasm::PageCount newPageCount) { jsMemory->growSuccessCallback(vm, oldPageCount, newPageCount); });
> > 
> > That's pretty cool!
> > 
> > The one downside is that it's hard to tell what each callback is for.  One way to make this easy to read is to introduce dummy enums for each WTF::Function:
> > 
> > enum NotifyLowMemory { NotifyLowMemoryTag }
> > enum SyncTryToReclaim { SyncTryToReclaimTag }
> > enum GrowMemory { GrowMemoryTag }
> > 
> > And then this would do:
> > 
> > RefPtr<...> memory = ...(...,
> >     [&vm] (NotifyLowMemory) { vm.heap.collectAsync(...); },
> >     [&vm] (SyncTryToRedlaim) ...
> > 
> > I don't feel strongly about it, but it might be nice.
> 
> Or you could just give all these lambdas names. That's usually what I do in
> this situation. The nice thing about Fil's suggestion is the type checker
> will catch a bug if you provide the wrong lambda.

Yeah I like the extra typecheck it gives.

> > Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.h:82
> > +    ImportFunctionInfo* importFunctionInfo(size_t importFunctionNum) { return &bitwise_cast<ImportFunctionInfo*>(bitwise_cast<char*>(this) + offsetOfTail())[importFunctionNum]; }
> > +    static size_t offsetOfTargetInstance(size_t importFunctionNum) { return offsetOfTail() + importFunctionNum * sizeof(ImportFunctionInfo) + OBJECT_OFFSETOF(ImportFunctionInfo, targetInstance); }
> > +    static size_t offsetOfWasmEntrypoint(size_t importFunctionNum) { return offsetOfTail() + importFunctionNum * sizeof(ImportFunctionInfo) + OBJECT_OFFSETOF(ImportFunctionInfo, wasmEntrypoint); }
> > +    static size_t offsetOfImportFunction(size_t importFunctionNum) { return offsetOfTail() + importFunctionNum * sizeof(ImportFunctionInfo) + OBJECT_OFFSETOF(ImportFunctionInfo, importFunction); }
> > +    JSObject* importFunction(unsigned importFunctionNum) { RELEASE_ASSERT(importFunctionNum < m_numImportFunctions); return importFunctionInfo(importFunctionNum)->importFunction.get(); }
> 
> oh boy

That'll go away :)

> > Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.cpp:124
> > +const Wasm::ModuleInformation& JSWebAssemblyModule::moduleInformation() const
> > +{
> > +    return m_module->moduleInformation();
> > +}
> > +
> > +SymbolTable* JSWebAssemblyModule::exportSymbolTable() const
> > +{
> > +    return m_exportSymbolTable.get();
> > +}
> > +
> > +Wasm::SignatureIndex JSWebAssemblyModule::signatureIndexFromFunctionIndexSpace(unsigned functionIndexSpace) const
> > +{
> > +    return m_module->signatureIndexFromFunctionIndexSpace(functionIndexSpace);
> > +}
> > +
> > +WebAssemblyToJSCallee* JSWebAssemblyModule::callee() const
> > +{
> > +    return m_callee.get();
> > +}
> > +
> > +JSWebAssemblyCodeBlock* JSWebAssemblyModule::codeBlock(Wasm::MemoryMode mode)
> > +{
> > +    return m_codeBlocks[static_cast<size_t>(mode)].get();
> > +}
> > +
> > +Wasm::Module& JSWebAssemblyModule::module()
> > +{
> > +    return m_module.get();
> > +}
> 
> these all seem like the should be inlined. Are they only called from the cpp
> file?

The problem is that JSWebAssemblyModule.h is included from WebCore for SerializedScriptValue.cpp, and I want to expose as few other headers as possible. Really I should have exported helpers that WebCore passes opaque pointers to, and hide all the details...

[WebKit Commit Bot]

Comment on attachment 322703 [details]
patch

Clearing flags on attachment: 322703

Committed r222873: <http://trac.webkit.org/changeset/222873>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34819035>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 178031

[JF Bastien]

Fixed when re-comitting #177473.

*** This bug has been marked as a duplicate of bug 177473 ***