Skip to content

Commit 7df1087

Browse files
geoffreygarengeoffreygaren
committed
Refactored the JSC::Heap extra cost API for clarity and to make some known bugs more obvious
https://bugs.webkit.org/show_bug.cgi?id=142589 Reviewed by Andreas Kling. Source/JavaScriptCore: * API/JSBase.cpp: (JSReportExtraMemoryCost): Added a FIXME to annotate a known bug. * bytecode/CodeBlock.cpp: (JSC::CodeBlock::CodeBlock): (JSC::CodeBlock::visitAggregate): * bytecode/CodeBlock.h: (JSC::CodeBlock::setJITCode): Updated for rename. * heap/Heap.cpp: (JSC::Heap::Heap): (JSC::Heap::reportExtraMemoryAllocatedSlowCase): (JSC::Heap::deprecatedReportExtraMemorySlowCase): Renamed our reporting APIs to clarify their relationship to each other: One must report extra memory at the time of allocation, and at the time the GC visits it. (JSC::Heap::extraMemorySize): (JSC::Heap::size): (JSC::Heap::capacity): (JSC::Heap::sizeAfterCollect): (JSC::Heap::willStartCollection): Updated for renames. Added explicit API for deprecated users who can't use our best API. (JSC::Heap::reportExtraMemoryCostSlowCase): Deleted. (JSC::Heap::extraSize): Deleted. * heap/Heap.h: * heap/HeapInlines.h: (JSC::Heap::reportExtraMemoryAllocated): (JSC::Heap::reportExtraMemoryVisited): (JSC::Heap::deprecatedReportExtraMemory): (JSC::Heap::reportExtraMemoryCost): Deleted. Ditto. * heap/SlotVisitor.h: * heap/SlotVisitorInlines.h: (JSC::SlotVisitor::reportExtraMemoryVisited): (JSC::SlotVisitor::reportExtraMemoryUsage): Deleted. Moved this functionality into the Heap since it's pretty detailed in its access to the heap. * runtime/JSArrayBufferView.cpp: (JSC::JSArrayBufferView::ConstructionContext::ConstructionContext): * runtime/JSGenericTypedArrayViewInlines.h: (JSC::JSGenericTypedArrayView<Adaptor>::visitChildren): Updated for renames. * runtime/JSString.cpp: (JSC::JSString::visitChildren): (JSC::JSRopeString::resolveRopeToAtomicString): (JSC::JSRopeString::resolveRope): * runtime/JSString.h: (JSC::JSString::finishCreation): Updated for renames. * runtime/SparseArrayValueMap.cpp: (JSC::SparseArrayValueMap::add): Added FIXME. * runtime/WeakMapData.cpp: (JSC::WeakMapData::visitChildren): Updated for rename. Source/WebCore: Updated for renames to JSC extra cost APIs. Added FIXMEs to our 10 use cases that are currently wrong, including canvas, which is the cause of https://bugs.webkit.org/show_bug.cgi?id=142457. * Modules/mediasource/SourceBuffer.cpp: (WebCore::SourceBuffer::appendBufferInternal): (WebCore::SourceBuffer::sourceBufferPrivateAppendComplete): (WebCore::SourceBuffer::reportExtraMemoryAllocated): (WebCore::SourceBuffer::reportExtraMemoryCost): Deleted. * Modules/mediasource/SourceBuffer.h: * bindings/js/JSDocumentCustom.cpp: (WebCore::toJS): * bindings/js/JSImageDataCustom.cpp: (WebCore::toJS): * bindings/js/JSNodeListCustom.cpp: (WebCore::createWrapper): * bindings/scripts/CodeGeneratorJS.pm: (GenerateImplementation): * dom/CollectionIndexCache.cpp: (WebCore::reportExtraMemoryAllocatedForCollectionIndexCache): (WebCore::reportExtraMemoryCostForCollectionIndexCache): Deleted. * dom/CollectionIndexCache.h: (WebCore::Iterator>::computeNodeCountUpdatingListCache): * html/HTMLCanvasElement.cpp: (WebCore::HTMLCanvasElement::createImageBuffer): * html/HTMLCollection.h: (WebCore::CollectionNamedElementCache::didPopulate): * html/HTMLImageLoader.cpp: (WebCore::HTMLImageLoader::imageChanged): * html/HTMLMediaElement.cpp: (WebCore::HTMLMediaElement::parseAttribute): * xml/XMLHttpRequest.cpp: (WebCore::XMLHttpRequest::dropProtection): Canonical link: https://commits.webkit.org/160626@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@181407 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 508c622 commit 7df1087

29 files changed

Lines changed: 237 additions & 87 deletions

‎Source/JavaScriptCore/API/JSBase.cpp‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,10 @@ void JSReportExtraMemoryCost(JSContextRef ctx, size_t size)
139139
}
140140
ExecState* exec = toJS(ctx);
141141
JSLockHolder locker(exec);
142-
exec->vm().heap.reportExtraMemoryCost(size);
142+
143+
// FIXME: switch to deprecatedReportExtraMemory.
144+
// https://bugs.webkit.org/show_bug.cgi?id=142593
145+
exec->vm().heap.reportExtraMemoryAllocated(size);
143146
}
144147

145148
extern "C" JS_EXPORT void JSSynchronousGarbageCollectForDebugging(JSContextRef);

‎Source/JavaScriptCore/ChangeLog‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,69 @@
1+
2015-03-11 Geoffrey Garen <ggaren@apple.com>
2+
3+
Refactored the JSC::Heap extra cost API for clarity and to make some known bugs more obvious
4+
https://bugs.webkit.org/show_bug.cgi?id=142589
5+
6+
Reviewed by Andreas Kling.
7+
8+
* API/JSBase.cpp:
9+
(JSReportExtraMemoryCost): Added a FIXME to annotate a known bug.
10+
11+
* bytecode/CodeBlock.cpp:
12+
(JSC::CodeBlock::CodeBlock):
13+
(JSC::CodeBlock::visitAggregate):
14+
* bytecode/CodeBlock.h:
15+
(JSC::CodeBlock::setJITCode): Updated for rename.
16+
17+
* heap/Heap.cpp:
18+
(JSC::Heap::Heap):
19+
(JSC::Heap::reportExtraMemoryAllocatedSlowCase):
20+
(JSC::Heap::deprecatedReportExtraMemorySlowCase): Renamed our reporting
21+
APIs to clarify their relationship to each other: One must report extra
22+
memory at the time of allocation, and at the time the GC visits it.
23+
24+
(JSC::Heap::extraMemorySize):
25+
(JSC::Heap::size):
26+
(JSC::Heap::capacity):
27+
(JSC::Heap::sizeAfterCollect):
28+
(JSC::Heap::willStartCollection): Updated for renames. Added explicit
29+
API for deprecated users who can't use our best API.
30+
31+
(JSC::Heap::reportExtraMemoryCostSlowCase): Deleted.
32+
(JSC::Heap::extraSize): Deleted.
33+
34+
* heap/Heap.h:
35+
* heap/HeapInlines.h:
36+
(JSC::Heap::reportExtraMemoryAllocated):
37+
(JSC::Heap::reportExtraMemoryVisited):
38+
(JSC::Heap::deprecatedReportExtraMemory):
39+
(JSC::Heap::reportExtraMemoryCost): Deleted. Ditto.
40+
41+
* heap/SlotVisitor.h:
42+
* heap/SlotVisitorInlines.h:
43+
(JSC::SlotVisitor::reportExtraMemoryVisited):
44+
(JSC::SlotVisitor::reportExtraMemoryUsage): Deleted. Moved this
45+
functionality into the Heap since it's pretty detailed in its access
46+
to the heap.
47+
48+
* runtime/JSArrayBufferView.cpp:
49+
(JSC::JSArrayBufferView::ConstructionContext::ConstructionContext):
50+
* runtime/JSGenericTypedArrayViewInlines.h:
51+
(JSC::JSGenericTypedArrayView<Adaptor>::visitChildren): Updated for
52+
renames.
53+
54+
* runtime/JSString.cpp:
55+
(JSC::JSString::visitChildren):
56+
(JSC::JSRopeString::resolveRopeToAtomicString):
57+
(JSC::JSRopeString::resolveRope):
58+
* runtime/JSString.h:
59+
(JSC::JSString::finishCreation): Updated for renames.
60+
61+
* runtime/SparseArrayValueMap.cpp:
62+
(JSC::SparseArrayValueMap::add): Added FIXME.
63+
64+
* runtime/WeakMapData.cpp:
65+
(JSC::WeakMapData::visitChildren): Updated for rename.
66+
167
2015-03-11 Ryosuke Niwa <rniwa@webkit.org>
268

369
Calling super() in a base class results in a crash

‎Source/JavaScriptCore/bytecode/CodeBlock.cpp‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1677,7 +1677,7 @@ CodeBlock::CodeBlock(CopyParsedBlockTag, CodeBlock& other)
16771677
}
16781678

16791679
m_heap->m_codeBlocks.add(this);
1680-
m_heap->reportExtraMemoryCost(sizeof(CodeBlock));
1680+
m_heap->reportExtraMemoryAllocated(sizeof(CodeBlock));
16811681
}
16821682

16831683
CodeBlock::CodeBlock(ScriptExecutable* ownerExecutable, UnlinkedCodeBlock* unlinkedCodeBlock, JSScope* scope, PassRefPtr<SourceProvider> sourceProvider, unsigned sourceOffset, unsigned firstLineColumnOffset)
@@ -2131,7 +2131,7 @@ CodeBlock::CodeBlock(ScriptExecutable* ownerExecutable, UnlinkedCodeBlock* unlin
21312131
dumpBytecode();
21322132

21332133
m_heap->m_codeBlocks.add(this);
2134-
m_heap->reportExtraMemoryCost(sizeof(CodeBlock) + m_instructions.size() * sizeof(Instruction));
2134+
m_heap->reportExtraMemoryAllocated(sizeof(CodeBlock) + m_instructions.size() * sizeof(Instruction));
21352135
}
21362136

21372137
CodeBlock::~CodeBlock()
@@ -2223,15 +2223,15 @@ void CodeBlock::visitAggregate(SlotVisitor& visitor)
22232223
if (CodeBlock* otherBlock = specialOSREntryBlockOrNull())
22242224
otherBlock->visitAggregate(visitor);
22252225

2226-
visitor.reportExtraMemoryUsage(ownerExecutable(), sizeof(CodeBlock));
2226+
visitor.reportExtraMemoryVisited(ownerExecutable(), sizeof(CodeBlock));
22272227
if (m_jitCode)
2228-
visitor.reportExtraMemoryUsage(ownerExecutable(), m_jitCode->size());
2228+
visitor.reportExtraMemoryVisited(ownerExecutable(), m_jitCode->size());
22292229
if (m_instructions.size()) {
22302230
// Divide by refCount() because m_instructions points to something that is shared
22312231
// by multiple CodeBlocks, and we only want to count it towards the heap size once.
22322232
// Having each CodeBlock report only its proportional share of the size is one way
22332233
// of accomplishing this.
2234-
visitor.reportExtraMemoryUsage(ownerExecutable(), m_instructions.size() * sizeof(Instruction) / m_instructions.refCount());
2234+
visitor.reportExtraMemoryVisited(ownerExecutable(), m_instructions.size() * sizeof(Instruction) / m_instructions.refCount());
22352235
}
22362236

22372237
visitor.append(&m_unlinkedCode);

‎Source/JavaScriptCore/bytecode/CodeBlock.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -273,7 +273,7 @@ class CodeBlock : public ThreadSafeRefCounted<CodeBlock>, public UnconditionalFi
273273
void setJITCode(PassRefPtr<JITCode> code)
274274
{
275275
ASSERT(m_heap->isDeferred());
276-
m_heap->reportExtraMemoryCost(code->size());
276+
m_heap->reportExtraMemoryAllocated(code->size());
277277
ConcurrentJITLocker locker(m_lock);
278278
WTF::storeStoreFence(); // This is probably not needed because the lock will also do something similar, but it's good to be paranoid.
279279
m_jitCode = code;

‎Source/JavaScriptCore/heap/Heap.cpp‎

Lines changed: 17 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -309,7 +309,8 @@ Heap::Heap(VM* vm, HeapType heapType)
309309
, m_operationInProgress(NoOperation)
310310
, m_objectSpace(this)
311311
, m_storageSpace(this)
312-
, m_extraMemoryUsage(0)
312+
, m_extraMemorySize(0)
313+
, m_deprecatedExtraMemorySize(0)
313314
, m_machineThreads(this)
314315
, m_sharedData(vm)
315316
, m_slotVisitor(m_sharedData)
@@ -392,23 +393,18 @@ void Heap::releaseDelayedReleasedObjects()
392393
#endif
393394
}
394395

395-
void Heap::reportExtraMemoryCostSlowCase(size_t cost)
396+
void Heap::reportExtraMemoryAllocatedSlowCase(size_t size)
396397
{
397-
// Our frequency of garbage collection tries to balance memory use against speed
398-
// by collecting based on the number of newly created values. However, for values
399-
// that hold on to a great deal of memory that's not in the form of other JS values,
400-
// that is not good enough - in some cases a lot of those objects can pile up and
401-
// use crazy amounts of memory without a GC happening. So we track these extra
402-
// memory costs. Only unusually large objects are noted, and we only keep track
403-
// of this extra cost until the next GC. In garbage collected languages, most values
404-
// are either very short lived temporaries, or have extremely long lifetimes. So
405-
// if a large value survives one garbage collection, there is not much point to
406-
// collecting more frequently as long as it stays alive.
407-
408-
didAllocate(cost);
398+
didAllocate(size);
409399
collectIfNecessaryOrDefer();
410400
}
411401

402+
void Heap::deprecatedReportExtraMemorySlowCase(size_t size)
403+
{
404+
m_deprecatedExtraMemorySize += size;
405+
reportExtraMemoryAllocatedSlowCase(size);
406+
}
407+
412408
void Heap::reportAbandonedObjectGraph()
413409
{
414410
// Our clients don't know exactly how much memory they
@@ -854,19 +850,19 @@ size_t Heap::objectCount()
854850
return m_objectSpace.objectCount();
855851
}
856852

857-
size_t Heap::extraSize()
853+
size_t Heap::extraMemorySize()
858854
{
859-
return m_extraMemoryUsage + m_arrayBuffers.size();
855+
return m_extraMemorySize + m_deprecatedExtraMemorySize + m_arrayBuffers.size();
860856
}
861857

862858
size_t Heap::size()
863859
{
864-
return m_objectSpace.size() + m_storageSpace.size() + extraSize();
860+
return m_objectSpace.size() + m_storageSpace.size() + extraMemorySize();
865861
}
866862

867863
size_t Heap::capacity()
868864
{
869-
return m_objectSpace.capacity() + m_storageSpace.capacity() + extraSize();
865+
return m_objectSpace.capacity() + m_storageSpace.capacity() + extraMemorySize();
870866
}
871867

872868
size_t Heap::sizeAfterCollect()
@@ -876,7 +872,7 @@ size_t Heap::sizeAfterCollect()
876872
// rather than all used (including dead) copied bytes, thus it's
877873
// always the case that m_totalBytesCopied <= m_storageSpace.size().
878874
ASSERT(m_totalBytesCopied <= m_storageSpace.size());
879-
return m_totalBytesVisited + m_totalBytesCopied + extraSize();
875+
return m_totalBytesVisited + m_totalBytesCopied + extraMemorySize();
880876
}
881877

882878
size_t Heap::protectedGlobalObjectCount()
@@ -1124,7 +1120,8 @@ void Heap::willStartCollection(HeapOperation collectionType)
11241120
}
11251121
if (m_operationInProgress == FullCollection) {
11261122
m_sizeBeforeLastFullCollect = m_sizeAfterLastCollect + m_bytesAllocatedThisCycle;
1127-
m_extraMemoryUsage = 0;
1123+
m_extraMemorySize = 0;
1124+
m_deprecatedExtraMemorySize = 0;
11281125

11291126
if (m_fullActivityCallback)
11301127
m_fullActivityCallback->willCollect();

‎Source/JavaScriptCore/heap/Heap.h‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -161,13 +161,21 @@ class Heap {
161161
JS_EXPORT_PRIVATE void collect(HeapOperation collectionType = AnyCollection);
162162
bool collectIfNecessaryOrDefer(); // Returns true if it did collect.
163163

164-
void reportExtraMemoryCost(size_t cost);
164+
// Use this API to report non-GC memory referenced by GC objects. Be sure to
165+
// call both of these functions: Calling only one may trigger catastropic
166+
// memory growth.
167+
void reportExtraMemoryAllocated(size_t);
168+
void reportExtraMemoryVisited(JSCell*, size_t);
169+
170+
// Use this API to report non-GC memory if you can't use the better API above.
171+
void deprecatedReportExtraMemory(size_t);
172+
165173
JS_EXPORT_PRIVATE void reportAbandonedObjectGraph();
166174

167175
JS_EXPORT_PRIVATE void protect(JSValue);
168176
JS_EXPORT_PRIVATE bool unprotect(JSValue); // True when the protect count drops to 0.
169177

170-
size_t extraSize(); // extra memory usage outside of pages allocated by the heap
178+
size_t extraMemorySize(); // Non-GC memory referenced by GC objects.
171179
JS_EXPORT_PRIVATE size_t size();
172180
JS_EXPORT_PRIVATE size_t capacity();
173181
JS_EXPORT_PRIVATE size_t objectCount();
@@ -261,15 +269,15 @@ class Heap {
261269
void* allocateWithoutDestructor(size_t); // For use with objects without destructors.
262270
template<typename ClassType> void* allocateObjectOfType(size_t); // Chooses one of the methods above based on type.
263271

264-
static const size_t minExtraCost = 256;
265-
static const size_t maxExtraCost = 1024 * 1024;
272+
static const size_t minExtraMemory = 256;
266273

267274
class FinalizerOwner : public WeakHandleOwner {
268275
virtual void finalize(Handle<Unknown>, void* context) override;
269276
};
270277

271278
JS_EXPORT_PRIVATE bool isValidAllocation(size_t);
272-
JS_EXPORT_PRIVATE void reportExtraMemoryCostSlowCase(size_t);
279+
JS_EXPORT_PRIVATE void reportExtraMemoryAllocatedSlowCase(size_t);
280+
JS_EXPORT_PRIVATE void deprecatedReportExtraMemorySlowCase(size_t);
273281

274282
void collectImpl(HeapOperation, void* stackOrigin, void* stackTop, MachineThreads::RegisterState&);
275283

@@ -353,7 +361,8 @@ class Heap {
353361
MarkedSpace m_objectSpace;
354362
CopiedSpace m_storageSpace;
355363
GCIncomingRefCountedSet<ArrayBuffer> m_arrayBuffers;
356-
size_t m_extraMemoryUsage;
364+
size_t m_extraMemorySize;
365+
size_t m_deprecatedExtraMemorySize;
357366

358367
HashSet<const JSCell*> m_copyingRememberedSet;
359368

‎Source/JavaScriptCore/heap/HeapInlines.h‎

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -152,10 +152,39 @@ inline void Heap::writeBarrier(const JSCell* from)
152152
#endif
153153
}
154154

155-
inline void Heap::reportExtraMemoryCost(size_t cost)
155+
inline void Heap::reportExtraMemoryAllocated(size_t size)
156156
{
157-
if (cost > minExtraCost)
158-
reportExtraMemoryCostSlowCase(cost);
157+
if (size > minExtraMemory)
158+
reportExtraMemoryAllocatedSlowCase(size);
159+
}
160+
161+
inline void Heap::reportExtraMemoryVisited(JSCell* owner, size_t size)
162+
{
163+
#if ENABLE(GGC)
164+
// We don't want to double-count the extra memory that was reported in previous collections.
165+
if (operationInProgress() == EdenCollection && Heap::isRemembered(owner))
166+
return;
167+
#else
168+
UNUSED_PARAM(owner);
169+
#endif
170+
171+
size_t* counter = &m_extraMemorySize;
172+
173+
#if ENABLE(COMPARE_AND_SWAP)
174+
for (;;) {
175+
size_t oldSize = *counter;
176+
if (WTF::weakCompareAndSwapSize(counter, oldSize, oldSize + size))
177+
return;
178+
}
179+
#else
180+
(*counter) += size;
181+
#endif
182+
}
183+
184+
inline void Heap::deprecatedReportExtraMemory(size_t size)
185+
{
186+
if (size > minExtraMemory)
187+
deprecatedReportExtraMemorySlowCase(size);
159188
}
160189

161190
template<typename Functor> inline typename Functor::ReturnType Heap::forEachProtectedCell(Functor& functor)

‎Source/JavaScriptCore/heap/SlotVisitor.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ class SlotVisitor {
105105

106106
void copyLater(JSCell*, CopyToken, void*, size_t);
107107

108-
void reportExtraMemoryUsage(JSCell* owner, size_t);
108+
void reportExtraMemoryVisited(JSCell* owner, size_t);
109109

110110
void addWeakReferenceHarvester(WeakReferenceHarvester*);
111111
void addUnconditionalFinalizer(UnconditionalFinalizer*);

‎Source/JavaScriptCore/heap/SlotVisitorInlines.h‎

Lines changed: 2 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -252,27 +252,9 @@ inline void SlotVisitor::copyLater(JSCell* owner, CopyToken token, void* ptr, si
252252
}
253253
}
254254

255-
inline void SlotVisitor::reportExtraMemoryUsage(JSCell* owner, size_t size)
255+
inline void SlotVisitor::reportExtraMemoryVisited(JSCell* owner, size_t size)
256256
{
257-
#if ENABLE(GGC)
258-
// We don't want to double-count the extra memory that was reported in previous collections.
259-
if (heap()->operationInProgress() == EdenCollection && Heap::isRemembered(owner))
260-
return;
261-
#else
262-
UNUSED_PARAM(owner);
263-
#endif
264-
265-
size_t* counter = &m_shared.m_vm->heap.m_extraMemoryUsage;
266-
267-
#if ENABLE(COMPARE_AND_SWAP)
268-
for (;;) {
269-
size_t oldSize = *counter;
270-
if (WTF::weakCompareAndSwapSize(counter, oldSize, oldSize + size))
271-
return;
272-
}
273-
#else
274-
(*counter) += size;
275-
#endif
257+
heap()->reportExtraMemoryVisited(owner, size);
276258
}
277259

278260
inline Heap* SlotVisitor::heap() const

‎Source/JavaScriptCore/runtime/JSArrayBufferView.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ JSArrayBufferView::ConstructionContext::ConstructionContext(
7878
return;
7979
}
8080

81-
vm.heap.reportExtraMemoryCost(static_cast<size_t>(length) * elementSize);
81+
vm.heap.reportExtraMemoryAllocated(static_cast<size_t>(length) * elementSize);
8282

8383
m_structure = structure;
8484
m_mode = OversizeTypedArray;

0 commit comments

Comments
 (0)