[webkit-reviews] review denied: [Bug 177473] WebAssembly: no VM / JS version of everything but Instance : [Attachment 321839] patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied JF Bastien <jfbastien at apple.com>'s
request for review:
Bug 177473: WebAssembly: no VM / JS version of everything but Instance
https://bugs.webkit.org/show_bug.cgi?id=177473

Attachment 321839: patch

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




--- Comment #19 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 321839
  --> https://bugs.webkit.org/attachment.cgi?id=321839
patch

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

I think it needs a few stylistic changes.

> Source/JavaScriptCore/interpreter/StackVisitor.cpp:155
> +    m_frame.m_CallerEntryFrame = m_frame.m_entryFrame;

Seems like Caller should be caller here.

> Source/JavaScriptCore/interpreter/StackVisitor.h:117
> +	   EntryFrame* m_CallerEntryFrame;

caller

> Source/JavaScriptCore/jit/AssemblyHelpers.h:370
> +    void copyCalleeSavesToEntryFrameCalleeSavesBuffer(EntryFrame**
topEntryFrame, const TempRegisterSet& usedRegisters = {
RegisterSet::stubUnavailableRegisters() })

Strongly recommend making this EntryFrame*&.  JSC is fuzzy about when we do *
and when we do &.  But this checks off some & boxes for me:

- Never null.
- Nobody is going to ever repoint this pointer.
- There is no existing convention that we should point to EntryFrame* instead
of referencing it (for example we prefer to point to ExecState rather than
reference it, therefore we almost universally say ExecState* to be consistent)

I don't know if these three rules are something we all agree upon, but it seems
that these three rules most easily explain JSC's existing uses of & and *.  So,
I think you should EntryFrame*& here.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:555
> +    int32_t (*growMemory)(JSWebAssemblyInstance*, int32_t) = []
(JSWebAssemblyInstance* instance, int32_t delta) -> int32_t {

I think this is an opportunity to say "auto growMemory" here instead.  I think
it's important to do that, because that means fewer churn if we decide to make
this capture things.

> Source/JavaScriptCore/wasm/WasmTable.cpp:104
> +    Checked newSizeChecked = size();
> +    newSizeChecked += delta;
> +    uint32_t newSize;
> +    if (newSizeChecked.safeGet(newSize) == CheckedState::DidOverflow)
> +	   return std::nullopt;
> +
> +    if (maximum() && newSize > *maximum())
> +	   return std::nullopt;
> +    if (!isValidSize(newSize))
> +	   return std::nullopt;
> +
> +    {
> +	   Checked reallocFunctionsSizeChecked = newSizeChecked;
> +	   reallocFunctionsSizeChecked *= sizeof(CallableFunction);
> +	   uint32_t reallocFunctionsSize;
> +	   if (reallocFunctionsSizeChecked.safeGet(reallocFunctionsSize) ==
CheckedState::DidOverflow)
> +	       return std::nullopt;
> +	   m_functions.realloc(reallocFunctionsSize);
> +	   for (uint32_t i = m_size; i < newSize; ++i)
> +	       new (&m_functions.get()[i]) Wasm::CallableFunction();
> +    }
> +    
> +    {
> +	   Checked reallocInstancesSizeChecked = newSizeChecked;
> +	   reallocInstancesSizeChecked *= sizeof(JSWebAssemblyInstance*);
> +	   uint32_t reallocInstancesSize;
> +	   if (reallocInstancesSizeChecked.safeGet(reallocInstancesSize) ==
CheckedState::DidOverflow)
> +	       return std::nullopt;
> +	   m_instances.realloc(reallocInstancesSize);
> +	   for (uint32_t i = m_size; i < newSize; ++i)
> +	       m_instances.get()[i] = nullptr;
> +    }
> +
> +    m_size = newSize;

Can all of this be Vector<>?

If not Vector<>, can you do something to reduce the deja vu?