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 15, 2024
1 parent f0b98a4 commit a301af9
Show file tree
Hide file tree
Showing 6 changed files with 82 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
4 changes: 4 additions & 0 deletions Source/WebKit/WebProcess/WebProcess.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1042,6 +1042,10 @@ void WebProcess::isJITEnabled(CompletionHandler<void(bool)>&& completionHandler)

void WebProcess::garbageCollectJavaScriptObjects()
{
{
JSLockHolder lock(commonVM());
commonVM().shrinkFootprintWhenIdle();
}
GCController::singleton().garbageCollectNow();
}

Expand Down

0 comments on commit a301af9

Please sign in to comment.