[webkit-reviews] review requested: [Bug 177887] WebAssembly: address no VM / JS follow-ups : [Attachment 322703] patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Oct 4 12:33:56 PDT 2017


JF Bastien <jfbastien at apple.com> has asked  for review:
Bug 177887: WebAssembly: address no VM / JS follow-ups
https://bugs.webkit.org/show_bug.cgi?id=177887

Attachment 322703: patch

https://bugs.webkit.org/attachment.cgi?id=322703&action=review




--- Comment #1 from JF Bastien <jfbastien at apple.com> ---
Created attachment 322703

  --> https://bugs.webkit.org/attachment.cgi?id=322703&action=review

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


More information about the webkit-reviews mailing list