WebKit Bugzilla
New
Browse
Search+
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
[patch]
the patch
blah.patch (text/plain), 19.33 KB, created by
Filip Pizlo
on 2018-01-27 20:42:00 PST
(
hide
)
Description:
the patch
Filename:
MIME Type:
Creator:
Filip Pizlo
Created:
2018-01-27 20:42:00 PST
Size:
19.33 KB
patch
obsolete
>Index: Source/JavaScriptCore/ChangeLog >=================================================================== >--- Source/JavaScriptCore/ChangeLog (revision 227717) >+++ Source/JavaScriptCore/ChangeLog (working copy) >@@ -1,3 +1,55 @@ >+2018-01-27 Filip Pizlo <fpizlo@apple.com> >+ >+ Make MarkedBlock::Footer bigger >+ https://bugs.webkit.org/show_bug.cgi?id=182220 >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ This makes the block footer larger by moving the newlyAllocated bits from the handle into >+ the footer. >+ >+ It used to be profitable to put anything we could into the handle because that would free up >+ payload space inside the block. But now that we want to use the footer for padding, it's >+ profitable to put GC state information - especially data that is used by the GC itself and so >+ is not useful for a Spectre attack - into the footer to increase object distancing. >+ >+ * heap/CellContainer.cpp: >+ (JSC::CellContainer::isNewlyAllocated const): >+ * heap/IsoCellSet.cpp: >+ (JSC::IsoCellSet::sweepToFreeList): >+ * heap/MarkedBlock.cpp: >+ (JSC::MarkedBlock::Handle::Handle): >+ (JSC::MarkedBlock::Footer::Footer): >+ (JSC::MarkedBlock::Handle::stopAllocating): >+ (JSC::MarkedBlock::Handle::lastChanceToFinalize): >+ (JSC::MarkedBlock::Handle::resumeAllocating): >+ (JSC::MarkedBlock::aboutToMarkSlow): >+ (JSC::MarkedBlock::resetAllocated): >+ (JSC::MarkedBlock::Handle::resetAllocated): Deleted. >+ * heap/MarkedBlock.h: >+ (JSC::MarkedBlock::newlyAllocatedVersion const): >+ (JSC::MarkedBlock::isNewlyAllocated): >+ (JSC::MarkedBlock::setNewlyAllocated): >+ (JSC::MarkedBlock::clearNewlyAllocated): >+ (JSC::MarkedBlock::newlyAllocated const): >+ (JSC::MarkedBlock::Handle::newlyAllocatedVersion const): Deleted. >+ (JSC::MarkedBlock::Handle::isNewlyAllocated): Deleted. >+ (JSC::MarkedBlock::Handle::setNewlyAllocated): Deleted. >+ (JSC::MarkedBlock::Handle::clearNewlyAllocated): Deleted. >+ (JSC::MarkedBlock::Handle::newlyAllocated const): Deleted. >+ * heap/MarkedBlockInlines.h: >+ (JSC::MarkedBlock::isNewlyAllocatedStale const): >+ (JSC::MarkedBlock::hasAnyNewlyAllocated): >+ (JSC::MarkedBlock::Handle::isLive): >+ (JSC::MarkedBlock::Handle::specializedSweep): >+ (JSC::MarkedBlock::Handle::newlyAllocatedMode): >+ (JSC::MarkedBlock::Handle::isNewlyAllocatedStale const): Deleted. >+ (JSC::MarkedBlock::Handle::hasAnyNewlyAllocated): Deleted. >+ * heap/MarkedSpace.cpp: >+ (JSC::MarkedSpace::endMarking): >+ * heap/SlotVisitor.cpp: >+ (JSC::SlotVisitor::appendJSCellOrAuxiliary): >+ > 2018-01-27 Filip Pizlo <fpizlo@apple.com> > > MarkedBlock should have a footer instead of a header >Index: Source/JavaScriptCore/heap/CellContainer.cpp >=================================================================== >--- Source/JavaScriptCore/heap/CellContainer.cpp (revision 227716) >+++ Source/JavaScriptCore/heap/CellContainer.cpp (working copy) >@@ -1,5 +1,5 @@ > /* >- * Copyright (C) 2016 Apple Inc. All rights reserved. >+ * Copyright (C) 2016-2018 Apple Inc. All rights reserved. > * > * Redistribution and use in source and binary forms, with or without > * modification, are permitted provided that the following conditions >@@ -34,9 +34,9 @@ bool CellContainer::isNewlyAllocated(Hea > { > if (isLargeAllocation()) > return largeAllocation().isNewlyAllocated(); >- MarkedBlock::Handle& handle = markedBlock().handle(); >- return !handle.isNewlyAllocatedStale() >- && handle.isNewlyAllocated(cell); >+ MarkedBlock& block = markedBlock(); >+ return !block.isNewlyAllocatedStale() >+ && block.isNewlyAllocated(cell); > } > > } // namespace JSC >Index: Source/JavaScriptCore/heap/IsoCellSet.cpp >=================================================================== >--- Source/JavaScriptCore/heap/IsoCellSet.cpp (revision 227716) >+++ Source/JavaScriptCore/heap/IsoCellSet.cpp (working copy) >@@ -126,8 +126,8 @@ void IsoCellSet::sweepToFreeList(MarkedB > RELEASE_ASSERT_NOT_REACHED(); > } > >- if (block->hasAnyNewlyAllocated()) { >- m_bits[block->index()]->concurrentFilter(block->newlyAllocated()); >+ if (block->block().hasAnyNewlyAllocated()) { >+ m_bits[block->index()]->concurrentFilter(block->block().newlyAllocated()); > return; > } > >Index: Source/JavaScriptCore/heap/MarkedBlock.cpp >=================================================================== >--- Source/JavaScriptCore/heap/MarkedBlock.cpp (revision 227717) >+++ Source/JavaScriptCore/heap/MarkedBlock.cpp (working copy) >@@ -62,7 +62,6 @@ MarkedBlock::Handle* MarkedBlock::tryCre > MarkedBlock::Handle::Handle(Heap& heap, AlignedMemoryAllocator* alignedMemoryAllocator, void* blockSpace) > : m_alignedMemoryAllocator(alignedMemoryAllocator) > , m_weakSet(heap.vm(), CellContainer()) >- , m_newlyAllocatedVersion(MarkedSpace::nullVersion) > { > m_block = new (NotNull, blockSpace) MarkedBlock(*heap.vm(), *this); > >@@ -101,6 +100,7 @@ MarkedBlock::Footer::Footer(VM& vm, Hand > : m_handle(handle) > , m_vm(&vm) > , m_markingVersion(MarkedSpace::nullVersion) >+ , m_newlyAllocatedVersion(MarkedSpace::nullVersion) > { > } > >@@ -144,12 +144,12 @@ void MarkedBlock::Handle::stopAllocating > // allocated from our free list are not currently marked, so we need another > // way to tell what's live vs dead. > >- m_newlyAllocated.clearAll(); >- m_newlyAllocatedVersion = heap()->objectSpace().newlyAllocatedVersion(); >+ blockFooter().m_newlyAllocated.clearAll(); >+ blockFooter().m_newlyAllocatedVersion = heap()->objectSpace().newlyAllocatedVersion(); > > forEachCell( > [&] (HeapCell* cell, HeapCell::Kind) -> IterationStatus { >- setNewlyAllocated(cell); >+ block().setNewlyAllocated(cell); > return IterationStatus::Continue; > }); > >@@ -159,7 +159,7 @@ void MarkedBlock::Handle::stopAllocating > dataLog("Free cell: ", RawPointer(cell), "\n"); > if (m_attributes.destruction == NeedsDestruction) > cell->zap(); >- clearNewlyAllocated(cell); >+ block().clearNewlyAllocated(cell); > }); > > m_isFreeListed = false; >@@ -173,8 +173,8 @@ void MarkedBlock::Handle::lastChanceToFi > block().clearHasAnyMarked(); > blockFooter().m_markingVersion = heap()->objectSpace().markingVersion(); > m_weakSet.lastChanceToFinalize(); >- m_newlyAllocated.clearAll(); >- m_newlyAllocatedVersion = heap()->objectSpace().newlyAllocatedVersion(); >+ blockFooter().m_newlyAllocated.clearAll(); >+ blockFooter().m_newlyAllocatedVersion = heap()->objectSpace().newlyAllocatedVersion(); > sweep(nullptr); > } > >@@ -188,7 +188,7 @@ void MarkedBlock::Handle::resumeAllocati > ASSERT(!directory()->isAllocated(NoLockingNecessary, this)); > ASSERT(!isFreeListed()); > >- if (!hasAnyNewlyAllocated()) { >+ if (!block().hasAnyNewlyAllocated()) { > if (false) > dataLog("There ain't no newly allocated.\n"); > // This means we had already exhausted the block when we stopped allocation. >@@ -236,7 +236,7 @@ void MarkedBlock::aboutToMarkSlow(HeapVe > if (false) > dataLog(RawPointer(this), ": Doing things.\n"); > HeapVersion newlyAllocatedVersion = space()->newlyAllocatedVersion(); >- if (handle().m_newlyAllocatedVersion == newlyAllocatedVersion) { >+ if (footer().m_newlyAllocatedVersion == newlyAllocatedVersion) { > // When do we get here? The block could not have been filled up. The newlyAllocated bits would > // have had to be created since the end of the last collection. The only things that create > // them are aboutToMarkSlow, lastChanceToFinalize, and stopAllocating. If it had been >@@ -244,11 +244,11 @@ void MarkedBlock::aboutToMarkSlow(HeapVe > // cannot be lastChanceToFinalize. So it must be stopAllocating. That means that we just > // computed the newlyAllocated bits just before the start of an increment. When we are in that > // mode, it seems as if newlyAllocated should subsume marks. >- ASSERT(handle().m_newlyAllocated.subsumes(footer().m_marks)); >+ ASSERT(footer().m_newlyAllocated.subsumes(footer().m_marks)); > footer().m_marks.clearAll(); > } else { >- handle().m_newlyAllocated.setAndClear(footer().m_marks); >- handle().m_newlyAllocatedVersion = newlyAllocatedVersion; >+ footer().m_newlyAllocated.setAndClear(footer().m_marks); >+ footer().m_newlyAllocatedVersion = newlyAllocatedVersion; > } > } > clearHasAnyMarked(); >@@ -259,10 +259,10 @@ void MarkedBlock::aboutToMarkSlow(HeapVe > directory->setIsMarkingNotEmpty(holdLock(directory->bitvectorLock()), &handle(), true); > } > >-void MarkedBlock::Handle::resetAllocated() >+void MarkedBlock::resetAllocated() > { >- m_newlyAllocated.clearAll(); >- m_newlyAllocatedVersion = MarkedSpace::nullVersion; >+ footer().m_newlyAllocated.clearAll(); >+ footer().m_newlyAllocatedVersion = MarkedSpace::nullVersion; > } > > void MarkedBlock::resetMarks() >Index: Source/JavaScriptCore/heap/MarkedBlock.h >=================================================================== >--- Source/JavaScriptCore/heap/MarkedBlock.h (revision 227717) >+++ Source/JavaScriptCore/heap/MarkedBlock.h (working copy) >@@ -174,18 +174,6 @@ public: > > bool isFreeListedCell(const void* target) const; > >- bool isNewlyAllocated(const void*); >- void setNewlyAllocated(const void*); >- void clearNewlyAllocated(const void*); >- const Bitmap<atomsPerBlock>& newlyAllocated() const; >- >- HeapVersion newlyAllocatedVersion() const { return m_newlyAllocatedVersion; } >- >- inline bool isNewlyAllocatedStale() const; >- >- inline bool hasAnyNewlyAllocated(); >- void resetAllocated(); >- > template <typename Functor> IterationStatus forEachCell(const Functor&); > template <typename Functor> inline IterationStatus forEachLiveCell(const Functor&); > template <typename Functor> inline IterationStatus forEachDeadCell(const Functor&); >@@ -232,8 +220,6 @@ public: > size_t m_atomsPerCell { std::numeric_limits<size_t>::max() }; > size_t m_endAtom { std::numeric_limits<size_t>::max() }; // This is a fuzzy end. Always test for < m_endAtom. > >- Bitmap<atomsPerBlock> m_newlyAllocated; >- > CellAttributes m_attributes; > bool m_isFreeListed { false }; > >@@ -242,8 +228,6 @@ public: > size_t m_index { std::numeric_limits<size_t>::max() }; > WeakSet m_weakSet; > >- HeapVersion m_newlyAllocatedVersion; >- > MarkedBlock* m_block { nullptr }; > }; > >@@ -296,8 +280,10 @@ public: > int16_t m_markCountBias; > > HeapVersion m_markingVersion; >+ HeapVersion m_newlyAllocatedVersion; > > Bitmap<atomsPerBlock> m_marks; >+ Bitmap<atomsPerBlock> m_newlyAllocated; > }; > > private: >@@ -330,6 +316,18 @@ public: > bool isAtom(const void*); > void clearMarked(const void*); > >+ bool isNewlyAllocated(const void*); >+ void setNewlyAllocated(const void*); >+ void clearNewlyAllocated(const void*); >+ const Bitmap<atomsPerBlock>& newlyAllocated() const; >+ >+ HeapVersion newlyAllocatedVersion() const { return footer().m_newlyAllocatedVersion; } >+ >+ inline bool isNewlyAllocatedStale() const; >+ >+ inline bool hasAnyNewlyAllocated(); >+ void resetAllocated(); >+ > size_t cellSize(); > const CellAttributes& attributes() const; > >@@ -584,24 +582,24 @@ inline const Bitmap<MarkedBlock::atomsPe > return footer().m_marks; > } > >-inline bool MarkedBlock::Handle::isNewlyAllocated(const void* p) >+inline bool MarkedBlock::isNewlyAllocated(const void* p) > { >- return m_newlyAllocated.get(m_block->atomNumber(p)); >+ return footer().m_newlyAllocated.get(atomNumber(p)); > } > >-inline void MarkedBlock::Handle::setNewlyAllocated(const void* p) >+inline void MarkedBlock::setNewlyAllocated(const void* p) > { >- m_newlyAllocated.set(m_block->atomNumber(p)); >+ footer().m_newlyAllocated.set(atomNumber(p)); > } > >-inline void MarkedBlock::Handle::clearNewlyAllocated(const void* p) >+inline void MarkedBlock::clearNewlyAllocated(const void* p) > { >- m_newlyAllocated.clear(m_block->atomNumber(p)); >+ footer().m_newlyAllocated.clear(atomNumber(p)); > } > >-inline const Bitmap<MarkedBlock::atomsPerBlock>& MarkedBlock::Handle::newlyAllocated() const >+inline const Bitmap<MarkedBlock::atomsPerBlock>& MarkedBlock::newlyAllocated() const > { >- return m_newlyAllocated; >+ return footer().m_newlyAllocated; > } > > inline bool MarkedBlock::isAtom(const void* p) >Index: Source/JavaScriptCore/heap/MarkedBlockInlines.h >=================================================================== >--- Source/JavaScriptCore/heap/MarkedBlockInlines.h (revision 227717) >+++ Source/JavaScriptCore/heap/MarkedBlockInlines.h (working copy) >@@ -40,12 +40,12 @@ inline unsigned MarkedBlock::Handle::cel > return MarkedSpace::blockPayload / cellSize(); > } > >-inline bool MarkedBlock::Handle::isNewlyAllocatedStale() const >+inline bool MarkedBlock::isNewlyAllocatedStale() const > { >- return m_newlyAllocatedVersion != space()->newlyAllocatedVersion(); >+ return footer().m_newlyAllocatedVersion != space()->newlyAllocatedVersion(); > } > >-inline bool MarkedBlock::Handle::hasAnyNewlyAllocated() >+inline bool MarkedBlock::hasAnyNewlyAllocated() > { > return !isNewlyAllocatedStale(); > } >@@ -143,19 +143,18 @@ ALWAYS_INLINE bool MarkedBlock::Handle:: > auto count = footer.m_lock.tryOptimisticFencelessRead(); > if (count.value) { > Dependency fenceBefore = Dependency::fence(count.input); >+ MarkedBlock& fencedBlock = *fenceBefore.consume(&block); >+ MarkedBlock::Footer& fencedFooter = fencedBlock.footer(); > MarkedBlock::Handle* fencedThis = fenceBefore.consume(this); > >- ASSERT(!fencedThis->isFreeListed()); >+ ASSERT_UNUSED(fencedThis, !fencedThis->isFreeListed()); > >- HeapVersion myNewlyAllocatedVersion = fencedThis->m_newlyAllocatedVersion; >+ HeapVersion myNewlyAllocatedVersion = fencedFooter.m_newlyAllocatedVersion; > if (myNewlyAllocatedVersion == newlyAllocatedVersion) { >- bool result = fencedThis->isNewlyAllocated(cell); >+ bool result = fencedBlock.isNewlyAllocated(cell); > if (footer.m_lock.fencelessValidate(count.value, Dependency::fence(result))) > return result; > } else { >- MarkedBlock& fencedBlock = *fenceBefore.consume(&block); >- MarkedBlock::Footer& fencedFooter = fencedBlock.footer(); >- > HeapVersion myMarkingVersion = fencedFooter.m_markingVersion; > if (myMarkingVersion != markingVersion > && (!isMarking || !fencedBlock.marksConveyLivenessDuringMarking(myMarkingVersion, markingVersion))) { >@@ -173,9 +172,9 @@ ALWAYS_INLINE bool MarkedBlock::Handle:: > > ASSERT(!isFreeListed()); > >- HeapVersion myNewlyAllocatedVersion = m_newlyAllocatedVersion; >+ HeapVersion myNewlyAllocatedVersion = footer.m_newlyAllocatedVersion; > if (myNewlyAllocatedVersion == newlyAllocatedVersion) >- return isNewlyAllocated(cell); >+ return block.isNewlyAllocated(cell); > > if (block.areMarksStale(markingVersion)) { > if (!isMarking) >@@ -326,7 +325,7 @@ void MarkedBlock::Handle::specializedSwe > for (size_t i = 0; i < m_endAtom; i += m_atomsPerCell) { > if (emptyMode == NotEmpty > && ((marksMode == MarksNotStale && footer.m_marks.get(i)) >- || (newlyAllocatedMode == HasNewlyAllocated && m_newlyAllocated.get(i)))) { >+ || (newlyAllocatedMode == HasNewlyAllocated && footer.m_newlyAllocated.get(i)))) { > isEmpty = false; > continue; > } >@@ -340,7 +339,7 @@ void MarkedBlock::Handle::specializedSwe > // We only want to discard the newlyAllocated bits if we're creating a FreeList, > // otherwise we would lose information on what's currently alive. > if (sweepMode == SweepToFreeList && newlyAllocatedMode == HasNewlyAllocated) >- m_newlyAllocatedVersion = MarkedSpace::nullVersion; >+ footer.m_newlyAllocatedVersion = MarkedSpace::nullVersion; > > if (space()->isMarking()) > footer.m_lock.unlock(); >@@ -464,7 +463,7 @@ inline MarkedBlock::Handle::ScribbleMode > > inline MarkedBlock::Handle::NewlyAllocatedMode MarkedBlock::Handle::newlyAllocatedMode() > { >- return hasAnyNewlyAllocated() ? HasNewlyAllocated : DoesNotHaveNewlyAllocated; >+ return block().hasAnyNewlyAllocated() ? HasNewlyAllocated : DoesNotHaveNewlyAllocated; > } > > inline MarkedBlock::Handle::MarksMode MarkedBlock::Handle::marksMode() >Index: Source/JavaScriptCore/heap/MarkedSpace.cpp >=================================================================== >--- Source/JavaScriptCore/heap/MarkedSpace.cpp (revision 227717) >+++ Source/JavaScriptCore/heap/MarkedSpace.cpp (working copy) >@@ -44,6 +44,11 @@ const Vector<size_t>& sizeClasses() > [] { > result = new Vector<size_t>(); > >+ if (Options::dumpSizeClasses()) { >+ dataLog("Block size: ", MarkedBlock::blockSize, "\n"); >+ dataLog("Footer size: ", sizeof(MarkedBlock::Footer), "\n"); >+ } >+ > auto add = [&] (size_t sizeClass) { > sizeClass = WTF::roundUpToMultipleOf<MarkedBlock::atomSize>(sizeClass); > if (Options::dumpSizeClasses()) >@@ -424,7 +429,7 @@ void MarkedSpace::endMarking() > if (UNLIKELY(nextVersion(m_newlyAllocatedVersion) == initialVersion)) { > forEachBlock( > [&] (MarkedBlock::Handle* handle) { >- handle->resetAllocated(); >+ handle->block().resetAllocated(); > }); > } > >Index: Source/JavaScriptCore/heap/SlotVisitor.cpp >=================================================================== >--- Source/JavaScriptCore/heap/SlotVisitor.cpp (revision 227716) >+++ Source/JavaScriptCore/heap/SlotVisitor.cpp (working copy) >@@ -1,5 +1,5 @@ > /* >- * Copyright (C) 2012-2017 Apple Inc. All rights reserved. >+ * Copyright (C) 2012-2018 Apple Inc. All rights reserved. > * > * Redistribution and use in source and binary forms, with or without > * modification, are permitted provided that the following conditions >@@ -170,8 +170,8 @@ void SlotVisitor::appendJSCellOrAuxiliar > out.print("Is marked raw: ", block.isMarkedRaw(jsCell), "\n"); > out.print("Marking version: ", block.markingVersion(), "\n"); > out.print("Heap marking version: ", heap()->objectSpace().markingVersion(), "\n"); >- out.print("Is newly allocated raw: ", block.handle().isNewlyAllocated(jsCell), "\n"); >- out.print("Newly allocated version: ", block.handle().newlyAllocatedVersion(), "\n"); >+ out.print("Is newly allocated raw: ", block.isNewlyAllocated(jsCell), "\n"); >+ out.print("Newly allocated version: ", block.newlyAllocatedVersion(), "\n"); > out.print("Heap newly allocated version: ", heap()->objectSpace().newlyAllocatedVersion(), "\n"); > } > UNREACHABLE_FOR_PLATFORM();
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=332487">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Flags:
jfbastien
:
review+
ews-watchlist
:
commit-queue-
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 182220
: 332487 |
332489
|
332490