Skip to content

Commit 5cd7df7

Browse files
committed
[MERGE #5099 @VSadov] Adding a configuration flag for setting the allocation policy limit.
Merge pull request #5099 from VSadov:allocLimit The flag is useful for testing purposes and for setting the policy limit in non-JSRT scenarios. - Fixed some missing `ReportExternalMemoryFree` calls. Not having those was tripping consistency asserts in memory policy manager in debug and could potentially result in false OOMs in release. Fixes:#5031
2 parents 4a342e0 + 0458adf commit 5cd7df7

File tree

7 files changed

+75
-25
lines changed

7 files changed

+75
-25
lines changed

lib/Common/ConfigFlagsList.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -535,6 +535,7 @@ PHASE(All)
535535
#define DEFAULT_CONFIG_LowMemoryCap (0xB900000) // 185 MB - based on memory cap for process on low-capacity device
536536
#define DEFAULT_CONFIG_NewPagesCapDuringBGSweeping (15000 * 4)
537537
#define DEFAULT_CONFIG_MaxSingleAllocSizeInMB (2048)
538+
#define DEFAULT_CONFIG_AllocationPolicyLimit (-1)
538539

539540
#define DEFAULT_CONFIG_MaxCodeFill (500)
540541
#define DEFAULT_CONFIG_MaxLoopsPerFunction (10)
@@ -1448,6 +1449,7 @@ FLAGNR(Boolean, RecyclerVerifyMark , "verify concurrent gc", false)
14481449
#endif
14491450
FLAGR (Number, LowMemoryCap , "Memory cap indicating a low-memory process", DEFAULT_CONFIG_LowMemoryCap)
14501451
FLAGNR(Number, NewPagesCapDuringBGSweeping, "New pages count allowed to be allocated during background sweeping", DEFAULT_CONFIG_NewPagesCapDuringBGSweeping)
1452+
FLAGR (Number, AllocPolicyLimit , "Memory allocation policy limit in MB (default: -1, which means no allocation policy limit).", DEFAULT_CONFIG_AllocationPolicyLimit)
14511453
#ifdef RUNTIME_DATA_COLLECTION
14521454
FLAGNR(String, RuntimeDataOutputFile, "Filename to write the dynamic profile info", nullptr)
14531455
#endif

lib/Common/Memory/AllocationPolicyManager.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,11 @@ typedef bool (__stdcall * PageAllocatorMemoryAllocationCallback)(__in LPVOID con
4141
context(NULL),
4242
memoryAllocationCallback(NULL)
4343
{
44+
Js::Number limitMB = Js::Configuration::Global.flags.AllocPolicyLimit;
45+
if (limitMB > 0)
46+
{
47+
memoryLimit = (size_t)limitMB * 1024 * 1024;
48+
}
4449
}
4550

4651
~AllocationPolicyManager()

lib/Common/Memory/Recycler.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2648,7 +2648,6 @@ bool Recycler::DoExternalAllocation(size_t size, ExternalAllocFunc externalAlloc
26482648
AutoExternalAllocation externalAllocation(this, size);
26492649
if (externalAllocFunc())
26502650
{
2651-
this->AddExternalMemoryUsage(size);
26522651
externalAllocation.allocationSucceeded = true;
26532652
return true;
26542653
}

lib/Runtime/Base/ThreadBoundThreadContextManager.cpp

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -33,11 +33,15 @@ ThreadContext * ThreadBoundThreadContextManager::EnsureContextForCurrentThread()
3333
// Just reinitialize the thread context.
3434
if (threadContext == nullptr)
3535
{
36-
threadContext = HeapNew(ThreadContext);
36+
bool requireConcurrencySupport = true;
37+
AllocationPolicyManager * policyManager = HeapNew(AllocationPolicyManager, requireConcurrencySupport);
38+
threadContext = HeapNew(ThreadContext, policyManager);
39+
3740
threadContext->SetIsThreadBound();
3841
if (!ThreadContextTLSEntry::TrySetThreadContext(threadContext))
3942
{
4043
HeapDelete(threadContext);
44+
HeapDelete(policyManager);
4145
return NULL;
4246
}
4347
}
@@ -181,17 +185,7 @@ void ThreadBoundThreadContextManager::DestroyAllContextsAndEntries(bool shouldDe
181185

182186
if (threadContext != nullptr)
183187
{
184-
#if DBG
185-
PageAllocator* pageAllocator = threadContext->GetPageAllocator();
186-
if (pageAllocator)
187-
{
188-
pageAllocator->SetConcurrentThreadId(::GetCurrentThreadId());
189-
}
190-
#endif
191-
192-
threadContext->ShutdownThreads();
193-
194-
HeapDelete(threadContext);
188+
ShutdownThreadContext(threadContext);
195189
}
196190

197191
if (currentThreadEntry != entry)
@@ -264,6 +258,12 @@ void ThreadContextManagerBase::ShutdownThreadContext(
264258

265259
if (deleteThreadContext)
266260
{
261+
AllocationPolicyManager * policyManager = threadContext->IsThreadBound() ? threadContext->GetAllocationPolicyManager() : nullptr;
267262
HeapDelete(threadContext);
263+
264+
if (policyManager)
265+
{
266+
HeapDelete(policyManager);
267+
}
268268
}
269269
}

lib/Runtime/Library/ArrayBuffer.cpp

Lines changed: 53 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -218,6 +218,11 @@ namespace Js
218218
{
219219
Assert(!this->isDetached);
220220

221+
// we are about to lose track of the buffer to another owner
222+
// report that we no longer own the memory
223+
Recycler* recycler = GetType()->GetLibrary()->GetRecycler();
224+
recycler->ReportExternalMemoryFree(bufferLength);
225+
221226
this->buffer = nullptr;
222227
this->bufferLength = 0;
223228
this->isDetached = true;
@@ -696,6 +701,13 @@ namespace Js
696701
Recycler* recycler = type->GetScriptContext()->GetRecycler();
697702
JavascriptArrayBuffer* result = RecyclerNewFinalized(recycler, JavascriptArrayBuffer, buffer, length, type);
698703
Assert(result);
704+
705+
// we take the ownership of the buffer and will have to free it so charge it to our quota.
706+
if (!recycler->RequestExternalMemoryAllocation(length))
707+
{
708+
JavascriptError::ThrowOutOfMemoryError(result->GetScriptContext());
709+
}
710+
699711
recycler->AddExternalMemoryUsage(length);
700712
return result;
701713
}
@@ -707,7 +719,6 @@ namespace Js
707719
{
708720
freeFunction(this->buffer);
709721
this->buffer = nullptr;
710-
this->recycler->ReportExternalMemoryFree(this->bufferLength);
711722
}
712723
this->bufferLength = 0;
713724
}
@@ -770,12 +781,6 @@ namespace Js
770781

771782
void JavascriptArrayBuffer::Finalize(bool isShutdown)
772783
{
773-
// In debugger scenario, ScriptAuthor can create scriptContext and delete scriptContext
774-
// explicitly. So for the builtin, while javascriptLibrary is still alive fine, the
775-
// matching scriptContext might have been deleted and the javascriptLibrary->scriptContext
776-
// field reset (but javascriptLibrary is still alive).
777-
// Use the recycler field off library instead of scriptcontext to avoid av.
778-
779784
// Recycler may not be available at Dispose. We need to
780785
// free the memory and report that it has been freed at the same
781786
// time. Otherwise, AllocationPolicyManager is unable to provide correct feedback
@@ -885,6 +890,12 @@ namespace Js
885890
if (buffer)
886891
{
887892
result = RecyclerNewFinalized(recycler, WebAssemblyArrayBuffer, buffer, length, type);
893+
894+
// we take the ownership of the buffer and will have to free it so charge it to our quota.
895+
if (!recycler->RequestExternalMemoryAllocation(length))
896+
{
897+
JavascriptError::ThrowOutOfMemoryError(result->GetScriptContext());
898+
}
888899
}
889900
else
890901
{
@@ -898,9 +909,9 @@ namespace Js
898909
{
899910
result = RecyclerNewFinalized(recycler, WebAssemblyArrayBuffer, length, type);
900911
}
901-
// Only add external memory when we create a new internal buffer
902-
recycler->AddExternalMemoryUsage(length);
903912
}
913+
914+
recycler->AddExternalMemoryUsage(length);
904915
Assert(result);
905916
return result;
906917
}
@@ -955,6 +966,11 @@ namespace Js
955966
{
956967
return nullptr;
957968
}
969+
970+
// We are transferring the buffer to the new owner.
971+
// To avoid double-charge to the allocation quota we will free the "diff" amount here.
972+
this->GetRecycler()->ReportExternalMemoryFree(growSize);
973+
958974
return finalizeGrowMemory(this->GetLibrary()->CreateWebAssemblyArrayBuffer(this->buffer, newBufferLength));
959975
}
960976
#endif
@@ -990,6 +1006,10 @@ namespace Js
9901006
return nullptr;
9911007
}
9921008

1009+
// We are transferring the buffer to the new owner.
1010+
// To avoid double-charge to the allocation quota we will free the "diff" amount here.
1011+
this->GetRecycler()->ReportExternalMemoryFree(growSize);
1012+
9931013
WebAssemblyArrayBuffer* newArrayBuffer = finalizeGrowMemory(this->GetLibrary()->CreateWebAssemblyArrayBuffer(newBuffer, newBufferLength));
9941014
// We've successfully Detached this buffer and created a new WebAssemblyArrayBuffer
9951015
autoDisableInterrupt.Completed();
@@ -1018,15 +1038,37 @@ namespace Js
10181038
ProjectionArrayBuffer* ProjectionArrayBuffer::Create(byte* buffer, uint32 length, DynamicType * type)
10191039
{
10201040
Recycler* recycler = type->GetScriptContext()->GetRecycler();
1041+
ProjectionArrayBuffer* result = RecyclerNewFinalized(recycler, ProjectionArrayBuffer, buffer, length, type);
1042+
1043+
// we take the ownership of the buffer and will have to free it so charge it to our quota.
1044+
if (!recycler->RequestExternalMemoryAllocation(length))
1045+
{
1046+
JavascriptError::ThrowOutOfMemoryError(result->GetScriptContext());
1047+
}
1048+
10211049
// This is user passed [in] buffer, user should AddExternalMemoryUsage before calling jscript, but
10221050
// I don't see we ask everyone to do this. Let's add the memory pressure here as well.
10231051
recycler->AddExternalMemoryUsage(length);
1024-
return RecyclerNewFinalized(recycler, ProjectionArrayBuffer, buffer, length, type);
1052+
return result;
1053+
10251054
}
10261055

1027-
void ProjectionArrayBuffer::Dispose(bool isShutdown)
1056+
void ProjectionArrayBuffer::Finalize(bool isShutdown)
10281057
{
10291058
CoTaskMemFree(buffer);
1059+
// Recycler may not be available at Dispose. We need to
1060+
// free the memory and report that it has been freed at the same
1061+
// time. Otherwise, AllocationPolicyManager is unable to provide correct feedback
1062+
Recycler* recycler = GetType()->GetLibrary()->GetRecycler();
1063+
recycler->ReportExternalMemoryFree(bufferLength);
1064+
1065+
buffer = nullptr;
1066+
bufferLength = 0;
1067+
}
1068+
1069+
void ProjectionArrayBuffer::Dispose(bool isShutdown)
1070+
{
1071+
/* See ProjectionArrayBuffer::Finalize */
10301072
}
10311073

10321074
ExternalArrayBuffer::ExternalArrayBuffer(byte *buffer, uint32 length, DynamicType *type)

lib/Runtime/Library/ArrayBuffer.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -325,7 +325,7 @@ namespace Js
325325
// take over ownership. a CoTaskMemAlloc'ed buffer passed in via projection.
326326
static ProjectionArrayBuffer* Create(byte* buffer, DECLSPEC_GUARD_OVERFLOW uint32 length, DynamicType * type);
327327
virtual void Dispose(bool isShutdown) override;
328-
virtual void Finalize(bool isShutdown) override {};
328+
virtual void Finalize(bool isShutdown) override;
329329
private:
330330
ProjectionArrayBuffer(uint32 length, DynamicType * type);
331331
ProjectionArrayBuffer(byte* buffer, uint32 length, DynamicType * type);

lib/Runtime/Library/SharedArrayBuffer.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -617,6 +617,8 @@ namespace Js
617617
{
618618
return false;
619619
}
620+
621+
this->GetRecycler()->AddExternalMemoryUsage(growSize);
620622
}
621623
else
622624
{

0 commit comments

Comments
 (0)