Skip to content

Commit

Permalink
[2.38] limit conservative scan range
Browse files Browse the repository at this point in the history
Turns out that conservative GC root scanning is going too deep into the stack
and this may prevent the collection of objects - see:

WebPlatformForEmbedded#1363

The change introduces 'stack guards' that prevent this in particular cases,
that is - if there is gloop main loop routine in the stack the GC root stack search
will not go deeper that this frame. This avoids some 'false positives' conservative roots
that can make the app hold a lot of memory.
  • Loading branch information
tomasz-karczewski-red committed Oct 28, 2024
1 parent f0b98a4 commit 1c94a4a
Show file tree
Hide file tree
Showing 5 changed files with 78 additions and 1 deletion.
3 changes: 2 additions & 1 deletion Source/JavaScriptCore/heap/MachineStackMarker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include <wtf/BitVector.h>
#include <wtf/PageBlock.h>
#include <wtf/StdLibExtras.h>
#include <wtf/ConservativeScanStackGuards.h>

namespace JSC {

Expand All @@ -44,7 +45,7 @@ void MachineThreads::gatherFromCurrentThread(ConservativeRoots& conservativeRoot
conservativeRoots.add(registersBegin, registersEnd, jitStubRoutines, codeBlocks);
}

conservativeRoots.add(currentThreadState.stackTop, currentThreadState.stackOrigin, jitStubRoutines, codeBlocks);
conservativeRoots.add(currentThreadState.stackTop, WTF::ConservativeScanStackGuards::updatedStackOrigin(currentThreadState.stackTop, currentThreadState.stackOrigin), jitStubRoutines, codeBlocks);
}

static inline int osRedZoneAdjustment()
Expand Down
2 changes: 2 additions & 0 deletions Source/WTF/wtf/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ set(WTF_PUBLIC_HEADERS
ConcurrentPtrHashSet.h
ConcurrentVector.h
Condition.h
ConservativeScanStackGuards.h
CountingLock.h
CrossThreadCopier.h
CrossThreadQueue.h
Expand Down Expand Up @@ -422,6 +423,7 @@ set(WTF_SOURCES
CompilationThread.cpp
ConcurrentBuffer.cpp
ConcurrentPtrHashSet.cpp
ConservativeScanStackGuards.cpp
CountingLock.cpp
CrossThreadCopier.cpp
CrossThreadTaskHandler.cpp
Expand Down
6 changes: 6 additions & 0 deletions Source/WTF/wtf/ConservativeScanStackGuards.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#include "ConservativeScanStackGuards.h"

namespace WTF {
std::atomic_bool ConservativeScanStackGuards::active {true};
thread_local std::deque<void*> ConservativeScanStackGuards::guard_ptr_stack;
}
65 changes: 65 additions & 0 deletions Source/WTF/wtf/ConservativeScanStackGuards.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
#pragma once

#include "Logging.h"

#include <atomic>
#include <deque>

namespace WTF {
class ConservativeScanStackGuards {
public:
struct ConservativeScanStackGuard {
ConservativeScanStackGuard() {
ConservativeScanStackGuards::setStackGuard(this);
}
~ConservativeScanStackGuard() {
ConservativeScanStackGuards::resetStackGuard(this);
}

private:
// Prevent heap allocation
void *operator new (size_t);
void *operator new[] (size_t);
void operator delete (void *);
void operator delete[] (void*);
};

friend struct ConservativeScanStackGuard;

static void* updatedStackOrigin(void *stackTop, void *stackOrigin) {
if (!active.load() || guard_ptr_stack.empty()) {
return stackOrigin;
}

void *ret = stackOrigin;

void *guard_ptr = guard_ptr_stack.back();

if (guard_ptr > stackTop && guard_ptr < stackOrigin) {
WTFLogAlways("ConservativeScanStackGuards: guard IN RANGE: stackTop: %p guard: %p stackOrigin: %p; correcting stackOrigin\n", stackTop, guard_ptr, stackOrigin);
ret = guard_ptr;
}
return ret;
}

static void setActive(bool act) {
active.store(act);
}

private:

static thread_local std::deque<void*> guard_ptr_stack;
static std::atomic_bool active;

static void setStackGuard(void *ptr) {
guard_ptr_stack.push_back(ptr);
}

static void resetStackGuard(void *ptr) {
if (ptr != guard_ptr_stack.back()) {
WTFLogAlways("ConservativeScanStackGuards::resetStackGuard expected %p, had: %p", guard_ptr_stack.back(), ptr);
}
guard_ptr_stack.pop_back();
}
};
}
3 changes: 3 additions & 0 deletions Source/WTF/wtf/glib/RunLoopGLib.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
#include <wtf/MainThread.h>
#include <wtf/glib/RunLoopSourcePriority.h>

#include "wtf/ConservativeScanStackGuards.h"

namespace WTF {

typedef struct {
Expand Down Expand Up @@ -103,6 +105,7 @@ void RunLoop::run()
ASSERT(!runLoop.m_mainLoops.isEmpty());

GMainLoop* innermostLoop = runLoop.m_mainLoops[0].get();
ConservativeScanStackGuards::ConservativeScanStackGuard guard;
if (!g_main_loop_is_running(innermostLoop)) {
g_main_context_push_thread_default(mainContext);
g_main_loop_run(innermostLoop);
Expand Down

0 comments on commit 1c94a4a

Please sign in to comment.