Strengthen WebAssemblyArrayBuffer.GrowMemory #3468
Conversation
| if (!RequestExternalMemoryAllocation(size)) | ||
| { | ||
| // Attempt to free some memory then try again | ||
| CollectNow<CollectOnTypedArrayAllocation>(); |
There was a problem hiding this comment.
Right now CollectOnTypedArrayAllocation is the only case we would use this allocation method. Should I make it generic anyway ?
lib/Runtime/Library/ArrayBuffer.cpp
Outdated
|
|
||
| AutoDiscardPTR<Js::ArrayBufferDetachedStateBase> state(DetachAndGetState()); | ||
| state->MarkAsClaimed(); | ||
| // Detach the buffer from this ArrayBuffer |
There was a problem hiding this comment.
Getting the state with DetachAndGetState then immediately calling state->MarkAsClaimed() was effectively only detaching the buffer.
So calling a version that detach, but doesn't create a state reduces overhead and increases readability
Cellule
force-pushed
the
users/micfer/wasm/grow_memory
branch
from
50acfef to
5156c9b
Compare
lib/Runtime/Library/ArrayBuffer.h
Outdated
| virtual BOOL GetDiagValueString(StringBuilder<ArenaAllocator>* stringBuilder, ScriptContext* requestContext) override; | ||
|
|
||
| virtual ArrayBufferDetachedStateBase* DetachAndGetState(); | ||
| void Detach(); |
There was a problem hiding this comment.
make it private member function as I don't see it is called externally. #Closed
There was a problem hiding this comment.
lib/Runtime/Library/ArrayBuffer.cpp
Outdated
| struct AutoFailFastOnBadState | ||
| { | ||
| bool isStateValid = true; | ||
| ~AutoFailFastOnBadState() |
There was a problem hiding this comment.
QueryContinue can throw javascript exception. In that case we fail fast?
Check AutoDisableInterrupt in threadcontext.h, which is suited for this need.
There was a problem hiding this comment.
I'm not sure I understand your point. What is the link between QueryContinue and WebAssembly.GrowMemory ?
After we've called ReAlloc, this->buffer is freed. Which means, any kind of exception before we detach the buffer will leave us in a bad state and there isn't much we can do to recover at that point.
This should be a very rare scenario, most of the time in very intense memory usage and thus it makes sense to failfast.
There was a problem hiding this comment.
QueryContinue is the service called from the host to interrupt on memory allocation. Upon querycontinue the interrupt service will determine if we have crossed the time-threshold. When that happens E_Abort exception will thrown. If you want to exclude the QueryContinue from the list of bad things could happen you need to disable that, otherwise it is fine.
In reply to: 131007291 [](ancestors = 131007291)
There was a problem hiding this comment.
Ok, so you mean, in order to minimize the kind of things that can throw here, I should use AutoDisableInterrupt when it becomes a bad state so it won't failfast.
Do I get this right ?
|
@akroshg can you take a look on my review fixes ? |
lib/Runtime/Library/ArrayBuffer.cpp
Outdated
| ReportDifferentialAllocation(newBufferLength, reportFailedFn); | ||
| if (failedReport) | ||
| // Disable QC while doing a ReAlloc to minimize chances to end up in a bad state | ||
| AutoDisableInterrupt autoDisableInterrupt(this->GetScriptContext()->GetThreadContext()->GetInterruptPoller(), true); |
There was a problem hiding this comment.
true [](start = 122, length = 4)
I think you need to pass false here - as you don't want explicit completion. True here means that you are going to call the autoDisableInterrupt.Completed() in this block.
There was a problem hiding this comment.
The true means that I want to disable Interrupt polling
AutoDisableInterrupt(InterruptPoller* interruptPoller, bool disable)
: interruptPoller(interruptPoller)
{
if (interruptPoller != nullptr)
{
previousState = interruptPoller->IsDisabled();
interruptPoller->SetDisabled(disable);
}
}
OS#12948195
Cellule
force-pushed
the
users/micfer/wasm/grow_memory
branch
from
7028755 to
49a6269
Compare
| } | ||
| } | ||
|
|
||
| void RequireExplicitCompletion() { m_explicitCompletion = true; } |
There was a problem hiding this comment.
nit: SetRequireExplicitCompletion
|
Interrupt use LGTM |
|
|
…owMemory Merge pull request #3468 from Cellule:users/micfer/wasm/grow_memory Make reporting external memory allocation cleaner with a wrapper and ensure we don't end up in a bad state after ReAlloc
…rrayBuffer.GrowMemory Merge pull request #3468 from Cellule:users/micfer/wasm/grow_memory Make reporting external memory allocation cleaner with a wrapper and ensure we don't end up in a bad state after ReAlloc
Make reporting external memory allocation cleaner with a wrapper and ensure we don't end up in a bad state after ReAlloc
This change is