Skip to content

Commit

Permalink
Backed out 3 changesets (bug 1716727) for causing build bustages. CLO…
Browse files Browse the repository at this point in the history
…SED TREE

Backed out changeset 11fb3262cccb (bug 1716727)
Backed out changeset 895ad6545236 (bug 1716727)
Backed out changeset c0fd4f429cd8 (bug 1716727)
  • Loading branch information
Butkovits Atila committed Jul 27, 2022
1 parent 4ec2a2d commit 1402637
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 150 deletions.
16 changes: 0 additions & 16 deletions memory/build/moz.build
Original file line number Diff line number Diff line change
Expand Up @@ -59,20 +59,4 @@ DisableStlWrapping()
if CONFIG["CC_TYPE"] == "clang-cl":
AllowCompilerWarnings() # workaround for bug 1090497

# Experimental: stall and retry on OOM (bug 1716727).
#
# This _will_ induce performance regressions in some (hopefully rare) contexts:
# we don't know whether any given allocation is supposed to be fallible, so we
# have to stall on any OOMed allocation.
#
# This is remediable, in theory. For now, though, we simply restrict it to
# Nightly, to give us enough data to determine whether this approach is worth
# the trouble of pursuing further.
#
# This experiment should probably not be active beyond 2022-09-01, and may be
# terminated early if the performance regression turns out to be worse than
# anticipated.
if CONFIG["NIGHTLY_BUILD"] and CONFIG["OS_TARGET"] == "WINNT":
DEFINES["MOZ_STALL_ON_OOM"] = True

REQUIRES_UNIFIED_BUILD = True
125 changes: 15 additions & 110 deletions memory/build/mozjemalloc.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1152,7 +1152,7 @@ class ArenaCollection {
// The main arena allows more dirty pages than the default for other arenas.
params.mMaxDirty = opt_dirty_max;
mDefaultArena =
mLock.Init() ? CreateArena(/* aIsPrivate = */ false, &params) : nullptr;
mLock.Init() ? CreateArena(/* IsPrivate = */ false, &params) : nullptr;
return bool(mDefaultArena);
}

Expand Down Expand Up @@ -1364,102 +1364,6 @@ static inline void ApplyZeroOrJunk(void* aPtr, size_t aSize) {
}
}

// Experiment under bug 1716727. (See ./moz.build for details.)
#ifdef XP_WIN

// Whether the current process should always stall, or only stall once.
static bool sShouldAlwaysStall = true;
MOZ_JEMALLOC_API void mozjemalloc_experiment_set_always_stall(bool aVal) {
sShouldAlwaysStall = aVal;
}

# ifdef MOZ_STALL_ON_OOM

// Implementation of VirtualAlloc wrapper (bug 1716727).
namespace MozAllocRetries {

// Maximum retry count on OOM.
constexpr size_t kMaxAttempts = 10;
// Minimum delay time between retries. (The actual delay time may be larger. See
// Microsoft's documentation for ::Sleep() for details.)
constexpr size_t kDelayMs = 50;

Atomic<bool> sHasStalled{false};
static bool ShouldStallAndRetry() {
if (sShouldAlwaysStall) {
return true;
}
// Otherwise, stall at most once.
return sHasStalled.compareExchange(false, true);
}

// Drop-in wrapper around VirtualAlloc. When out of memory, may attempt to stall
// and retry rather than returning immediately, in hopes that the page file is
// about to be expanded by Windows.
//
// Ref: https://docs.microsoft.com/en-us/troubleshoot/windows-client/performance/slow-page-file-growth-memory-allocation-errors
[[nodiscard]] void* MozVirtualAlloc(LPVOID lpAddress, SIZE_T dwSize,
DWORD flAllocationType, DWORD flProtect) {
constexpr auto IsOOMError = [] {
switch (::GetLastError()) {
// This is the usual error result from VirtualAlloc for OOM.
case ERROR_COMMITMENT_LIMIT:
// Although rare, this has also been observed in low-memory situations.
// (Presumably this means Windows can't allocate enough kernel-side space
// for its own internal representation of the process's virtual address
// space.)
case ERROR_NOT_ENOUGH_MEMORY:
return true;
}
return false;
};

{
void* ptr = ::VirtualAlloc(lpAddress, dwSize, flAllocationType, flProtect);
if (MOZ_LIKELY(ptr)) return ptr;

// We can't do anything for errors other than OOM...
if (!IsOOMError()) return nullptr;
// ... or if this wasn't a request to commit memory in the first place.
// (This function has no strategy for resolving MEM_RESERVE failures.)
if (!(flAllocationType & MEM_COMMIT)) return nullptr;
}

// Also return if we just aren't supposed to be retrying at the moment, for
// whatever reason.
if (!ShouldStallAndRetry()) return nullptr;

// Otherwise, retry.
for (size_t i = 0; i < kMaxAttempts; ++i) {
::Sleep(kDelayMs);
void* ptr = ::VirtualAlloc(lpAddress, dwSize, flAllocationType, flProtect);

if (ptr) {
// The OOM status has been handled, and should not be reported to
// telemetry.
if (IsOOMError()) {
::SetLastError(0);
}
return ptr;
}

// Failure for some reason other than OOM.
if (!IsOOMError()) {
return nullptr;
}
}

// Ah, well. We tried.
return nullptr;
}
} // namespace MozAllocRetries

using MozAllocRetries::MozVirtualAlloc;
# else
using MozVirtualAlloc = VirtualAlloc;
# endif // MOZ_STALL_ON_OOM
#endif // XP_WIN

// ***************************************************************************

static inline void pages_decommit(void* aAddr, size_t aSize) {
Expand Down Expand Up @@ -1509,7 +1413,7 @@ static inline void pages_decommit(void* aAddr, size_t aSize) {
// time, we may touch any region in chunksized increments.
size_t pages_size = std::min(aSize, kChunkSize - GetChunkOffsetForPtr(aAddr));
while (aSize > 0) {
if (!MozVirtualAlloc(aAddr, pages_size, MEM_COMMIT, PAGE_READWRITE)) {
if (!VirtualAlloc(aAddr, pages_size, MEM_COMMIT, PAGE_READWRITE)) {
return false;
}
aAddr = (void*)((uintptr_t)aAddr + pages_size);
Expand Down Expand Up @@ -1654,7 +1558,7 @@ using UniqueBaseNode =

static void* pages_map(void* aAddr, size_t aSize) {
void* ret = nullptr;
ret = MozVirtualAlloc(aAddr, aSize, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE);
ret = VirtualAlloc(aAddr, aSize, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE);
return ret;
}

Expand Down Expand Up @@ -1853,7 +1757,7 @@ void* AddressRadixTree<Bits>::Get(void* aKey) {
template <size_t Bits>
bool AddressRadixTree<Bits>::Set(void* aKey, void* aValue) {
MutexAutoLock lock(mLock);
void** slot = GetSlot(aKey, /* aCreate = */ true);
void** slot = GetSlot(aKey, /* create = */ true);
if (slot) {
*slot = aValue;
}
Expand All @@ -1865,11 +1769,11 @@ bool AddressRadixTree<Bits>::Set(void* aKey, void* aValue) {

// Return the offset between a and the nearest aligned address at or below a.
#define ALIGNMENT_ADDR2OFFSET(a, alignment) \
((size_t)((uintptr_t)(a) & ((alignment)-1)))
((size_t)((uintptr_t)(a) & (alignment - 1)))

// Return the smallest alignment multiple that is >= s.
#define ALIGNMENT_CEILING(s, alignment) \
(((s) + ((alignment)-1)) & (~((alignment)-1)))
(((s) + (alignment - 1)) & (~(alignment - 1)))

static void* pages_trim(void* addr, size_t alloc_size, size_t leadsize,
size_t size) {
Expand Down Expand Up @@ -2049,7 +1953,7 @@ static void* chunk_recycle(size_t aSize, size_t aAlignment, bool* aZeroed) {
// awkward to recycle allocations of varying sizes. Therefore we only allow
// recycling when the size equals the chunksize, unless deallocation is entirely
// disabled.
# define CAN_RECYCLE(size) ((size) == kChunkSize)
# define CAN_RECYCLE(size) (size == kChunkSize)
#else
# define CAN_RECYCLE(size) true
#endif
Expand Down Expand Up @@ -2225,7 +2129,7 @@ static inline arena_t* thread_local_arena(bool enabled) {
// because in practice nothing actually calls this function
// with `false`, except maybe at shutdown.
arena =
gArenas.CreateArena(/* aIsPrivate = */ false, /* aParams = */ nullptr);
gArenas.CreateArena(/* IsPrivate = */ false, /* Params = */ nullptr);
} else {
arena = gArenas.GetDefault();
}
Expand Down Expand Up @@ -4087,6 +3991,7 @@ static size_t GetKernelPageSize() {
static bool malloc_init_hard() {
unsigned i;
const char* opts;
long result;

AutoLock<StaticMutex> lock(gInitLock);

Expand All @@ -4101,18 +4006,18 @@ static bool malloc_init_hard() {
}

// Get page size and number of CPUs
const size_t result = GetKernelPageSize();
result = GetKernelPageSize();
// We assume that the page size is a power of 2.
MOZ_ASSERT(((result - 1) & result) == 0);
#ifdef MALLOC_STATIC_PAGESIZE
if (gPageSize % result) {
if (gPageSize % (size_t)result) {
_malloc_message(
_getprogname(),
"Compile-time page size does not divide the runtime one.\n");
MOZ_CRASH();
}
#else
gRealPageSize = gPageSize = result;
gRealPageSize = gPageSize = (size_t)result;
#endif

// Get runtime configuration.
Expand Down Expand Up @@ -4290,7 +4195,7 @@ inline void* BaseAllocator::malloc(size_t aSize) {
aSize = 1;
}
arena = mArena ? mArena : choose_arena(aSize);
ret = arena->Malloc(aSize, /* aZero = */ false);
ret = arena->Malloc(aSize, /* zero = */ false);

RETURN:
if (!ret) {
Expand Down Expand Up @@ -4327,7 +4232,7 @@ inline void* BaseAllocator::calloc(size_t aNum, size_t aSize) {
allocSize = 1;
}
arena_t* arena = mArena ? mArena : choose_arena(allocSize);
ret = arena->Malloc(allocSize, /* aZero = */ true);
ret = arena->Malloc(allocSize, /* zero = */ true);
} else {
ret = nullptr;
}
Expand Down Expand Up @@ -4361,7 +4266,7 @@ inline void* BaseAllocator::realloc(void* aPtr, size_t aSize) {
ret = nullptr;
} else {
arena_t* arena = mArena ? mArena : choose_arena(aSize);
ret = arena->Malloc(aSize, /* aZero = */ false);
ret = arena->Malloc(aSize, /* zero = */ false);
}
}

Expand Down
8 changes: 0 additions & 8 deletions memory/build/mozmemory.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
#include "mozilla/Attributes.h"
#include "mozilla/Types.h"
#include "mozjemalloc_types.h"
#include "stdbool.h"

#ifdef MOZ_MEMORY
// On OSX, malloc/malloc.h contains the declaration for malloc_good_size,
Expand Down Expand Up @@ -61,13 +60,6 @@ static inline void jemalloc_stats(jemalloc_stats_t* aStats) {
}
# endif

// Temporary configurator for experiment associated with bug 1716727.
# if defined(XP_WIN)
MOZ_JEMALLOC_API void mozjemalloc_experiment_set_always_stall(bool);
# else
static inline void mozjemalloc_experiment_set_always_stall(bool){};
# endif

#endif // MOZ_MEMORY

#define NOTHROW_MALLOC_DECL(name, return_type, ...) \
Expand Down
23 changes: 7 additions & 16 deletions toolkit/xre/nsEmbedFunctions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -274,23 +274,14 @@ void XRE_SetProcessType(const char* aProcessTypeString) {
}
called = true;

sChildProcessType = [&] {
for (GeckoProcessType t :
MakeEnumeratedRange(GeckoProcessType::GeckoProcessType_End)) {
if (!strcmp(XRE_GeckoProcessTypeToString(t), aProcessTypeString)) {
return t;
}
sChildProcessType = GeckoProcessType_Invalid;
for (GeckoProcessType t :
MakeEnumeratedRange(GeckoProcessType::GeckoProcessType_End)) {
if (!strcmp(XRE_GeckoProcessTypeToString(t), aProcessTypeString)) {
sChildProcessType = t;
return;
}
return GeckoProcessType_Invalid;
}();

// For the parent process, we're probably willing to accept an apparent
// lockup in preference to a crash. Always stall and retry.
//
// For child processes, an obvious OOM-crash may be preferable to slow
// performance. Retry at most once per process, then give up.
mozjemalloc_experiment_set_always_stall(sChildProcessType ==
GeckoProcessType_Default);
}
}

#if defined(XP_WIN)
Expand Down

0 comments on commit 1402637

Please sign in to comment.