Skip to content

Commit

Permalink
Suppress HeapSnapshot logging in some JSC stress tests.
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=260743
rdar://114474559

Reviewed by Justin Michaud.

Specifically, heap-analyzer-taking-lock.js and collect-continuously-should-not-wake-concurrent-collector-after-prevent-collection-is-called.js.
The logging is not needed for the tests, and the verbosity of the logging makes it hard to see real failures when running the JSC stress tests.

* JSTests/stress/collect-continuously-should-not-wake-concurrent-collector-after-prevent-collection-is-called.js:
* JSTests/stress/heap-analyzer-taking-lock.js:
* Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:
(JSC::HeapSnapshotBuilder::analyzeEdge):
(JSC::HeapSnapshotBuilder::json):
* Source/JavaScriptCore/runtime/OptionsList.h:

Canonical link: https://commits.webkit.org/267306@main
  • Loading branch information
Mark Lam committed Aug 25, 2023
1 parent 6ece913 commit 12ec99a
Show file tree
Hide file tree
Showing 4 changed files with 19 additions and 9 deletions.
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//@ runDefault("--maxPerThreadStackUsage=1572864", "--forceMiniVMMode=1", "--stealEmptyBlocksFromOtherAllocators=0", "--collectContinuously=1", "--watchdog=3000", "--watchdog-exception-ok")
//@ runDefault("--maxPerThreadStackUsage=1572864", "--forceMiniVMMode=1", "--stealEmptyBlocksFromOtherAllocators=0", "--collectContinuously=1", "--watchdog=3000", "--watchdog-exception-ok", "--verboseHeapSnapshotLogging=0")
// Reproducing the crash with this test is very hard. You should execute something like this.
// while true; do for x in {0..4}; do DYLD_FRAMEWORK_PATH=$VM $VM/jsc --maxPerThreadStackUsage=1572864 --forceMiniVMMode=1 --stealEmptyBlocksFromOtherAllocators=0 --collectContinuously=1 collect-continuously-should-not-wake-concurrent-collector-after-prevent-collection-is-called.js --watchdog=3000&; done; wait; sleep 0.1; done

Expand Down
2 changes: 2 additions & 0 deletions JSTests/stress/heap-analyzer-taking-lock.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
//@ requireOptions("--verboseHeapSnapshotLogging=0")

// Should not crash.
let array = [];
for (let i = 0; i < 10000; i++) {
Expand Down
23 changes: 15 additions & 8 deletions Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2016-2021 Apple Inc. All rights reserved.
* Copyright (C) 2016-2023 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
Expand Down Expand Up @@ -109,8 +109,10 @@ void HeapSnapshotBuilder::analyzeEdge(JSCell* from, JSCell* to, RootMarkReason r
Locker locker { m_buildingEdgeMutex };

if (m_snapshotType == SnapshotType::GCDebuggingSnapshot && !from) {
if (rootMarkReason == RootMarkReason::None && m_snapshotType == SnapshotType::GCDebuggingSnapshot)
WTFLogAlways("Cell %p is a root but no root marking reason was supplied", to);
if (rootMarkReason == RootMarkReason::None && m_snapshotType == SnapshotType::GCDebuggingSnapshot) {
if (Options::verboseHeapSnapshotLogging())
WTFLogAlways("Cell %p is a root but no root marking reason was supplied", to);
}

m_rootData.ensure(to, [] () -> RootData {
return { };
Expand Down Expand Up @@ -499,8 +501,10 @@ String HeapSnapshotBuilder::json(Function<bool (const HeapSnapshotNode&)> allowN
else {
auto fromLookup = allowedNodeIdentifiers.find(edge.from.cell);
if (fromLookup == allowedNodeIdentifiers.end()) {
if (m_snapshotType == SnapshotType::GCDebuggingSnapshot)
WTFLogAlways("Failed to find node for from-edge cell %p", edge.from.cell);
if (m_snapshotType == SnapshotType::GCDebuggingSnapshot) {
if (Options::verboseHeapSnapshotLogging())
WTFLogAlways("Failed to find node for from-edge cell %p", edge.from.cell);
}
return true;
}
edge.from.identifier = fromLookup->value;
Expand All @@ -511,8 +515,10 @@ String HeapSnapshotBuilder::json(Function<bool (const HeapSnapshotNode&)> allowN
else {
auto toLookup = allowedNodeIdentifiers.find(edge.to.cell);
if (toLookup == allowedNodeIdentifiers.end()) {
if (m_snapshotType == SnapshotType::GCDebuggingSnapshot)
WTFLogAlways("Failed to find node for to-edge cell %p", edge.to.cell);
if (m_snapshotType == SnapshotType::GCDebuggingSnapshot) {
if (Options::verboseHeapSnapshotLogging())
WTFLogAlways("Failed to find node for to-edge cell %p", edge.to.cell);
}
return true;
}
edge.to.identifier = toLookup->value;
Expand Down Expand Up @@ -563,7 +569,8 @@ String HeapSnapshotBuilder::json(Function<bool (const HeapSnapshotNode&)> allowN
for (auto it : m_rootData) {
auto snapshotNode = snapshot->nodeForCell(it.key);
if (!snapshotNode) {
WTFLogAlways("Failed to find snapshot node for cell %p", it.key);
if (Options::verboseHeapSnapshotLogging())
WTFLogAlways("Failed to find snapshot node for cell %p", it.key);
continue;
}

Expand Down
1 change: 1 addition & 0 deletions Source/JavaScriptCore/runtime/OptionsList.h
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ bool canUseWebAssemblyFastMemory();
v(Bool, forceFencedBarrier, false, Normal, nullptr) \
v(Bool, verboseVisitRace, false, Normal, nullptr) \
v(Bool, optimizeParallelSlotVisitorsForStoppedMutator, false, Normal, nullptr) \
v(Bool, verboseHeapSnapshotLogging, true, Normal, nullptr) \
v(Unsigned, largeHeapSize, 32 * 1024 * 1024, Normal, nullptr) \
v(Unsigned, smallHeapSize, 1 * 1024 * 1024, Normal, nullptr) \
v(Double, smallHeapRAMFraction, 0.25, Normal, nullptr) \
Expand Down

0 comments on commit 12ec99a

Please sign in to comment.