Before deleting a MarkedBlock we do not need to clear its m_directory pointer.
Created attachment 426246 [details] Patch
Comment on attachment 426246 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426246&action=review > Source/JavaScriptCore/ChangeLog:11 > + This patch adds prevents this uncessary store to hopefully reduce the number /adds prevents/prevents/ > Source/JavaScriptCore/heap/MarkedBlock.cpp:-79 > - removeFromDirectory(); Why is this ok? Wouldn't this leave the directory in a stale state?
Comment on attachment 426246 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426246&action=review >> Source/JavaScriptCore/heap/MarkedBlock.cpp:-79 >> - removeFromDirectory(); > > Why is this ok? Wouldn't this leave the directory in a stale state? Well, it's unnecessary. The only place we destruct handles (MarkedSpace::freeBlock) already removes the MarkedBlock from the directory before calling delete.
Comment on attachment 426246 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426246&action=review >> Source/JavaScriptCore/ChangeLog:11 >> + This patch adds prevents this uncessary store to hopefully reduce the number > > /adds prevents/prevents/ fixed. >>> Source/JavaScriptCore/heap/MarkedBlock.cpp:-79 >>> - removeFromDirectory(); >> >> Why is this ok? Wouldn't this leave the directory in a stale state? > > Well, it's unnecessary. The only place we destruct handles (MarkedSpace::freeBlock) already removes the MarkedBlock from the directory before calling delete. Well, it's unnecessary. The only place we destruct handles (MarkedSpace::freeBlock) already removes the MarkedBlock from the directory before calling delete.
Comment on attachment 426246 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426246&action=review > Source/JavaScriptCore/ChangeLog:10 > + paging in the footer from disk, which some data we have seen shows is happening. from disk or from the compressor > Source/JavaScriptCore/ChangeLog:11 > + This patch adds prevents this uncessary store to hopefully reduce the number adds prevents => prevents > Source/JavaScriptCore/ChangeLog:12 > + of pageins caused by Safari web content on Mac. compressor effect pertains to iOS too > Source/JavaScriptCore/heap/BlockDirectory.cpp:145 > + removeBlockForDeletion(block); Surprising to say "for deletion" when we are not deleting. How about instead adding an enum class WillDeleteBlock { Yes, No } parameter to removeBlock() that defaults to WillDeleteBlock::No? Then removeBlock() can call didRemoveFromDirectory if willDeleteBlock == WillDeleteBlock::YES. > Source/JavaScriptCore/heap/MarkedBlock.cpp:-79 > - removeFromDirectory(); Is this OK? Why?
Comment on attachment 426246 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426246&action=review >>>>> Source/JavaScriptCore/heap/MarkedBlock.cpp:-79 >>>>> - removeFromDirectory(); >>>> >>>> Why is this ok? Wouldn't this leave the directory in a stale state? >>> >>> Well, it's unnecessary. The only place we destruct handles (MarkedSpace::freeBlock) already removes the MarkedBlock from the directory before calling delete. >> >> Well, it's unnecessary. The only place we destruct handles (MarkedSpace::freeBlock) already removes the MarkedBlock from the directory before calling delete. > > Is this OK? Why? Only MarkedSpace::freeBlock calls this destructor, and it is calling removeBlockForDeletion. So, this does nothing in practice. When calling it, m_directory was always nullptr.
Comment on attachment 426246 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426246&action=review >> Source/JavaScriptCore/heap/BlockDirectory.cpp:145 >> + removeBlockForDeletion(block); > > Surprising to say "for deletion" when we are not deleting. > > How about instead adding an enum class WillDeleteBlock { Yes, No } parameter to removeBlock() that defaults to WillDeleteBlock::No? Then removeBlock() can call didRemoveFromDirectory if willDeleteBlock == WillDeleteBlock::YES. Sure, that's fine. >>>>>> Source/JavaScriptCore/heap/MarkedBlock.cpp:-79 >>>>>> - removeFromDirectory(); >>>>> >>>>> Why is this ok? Wouldn't this leave the directory in a stale state? >>>> >>>> Well, it's unnecessary. The only place we destruct handles (MarkedSpace::freeBlock) already removes the MarkedBlock from the directory before calling delete. >>> >>> Well, it's unnecessary. The only place we destruct handles (MarkedSpace::freeBlock) already removes the MarkedBlock from the directory before calling delete. >> >> Is this OK? Why? > > Only MarkedSpace::freeBlock calls this destructor, and it is calling removeBlockForDeletion. > So, this does nothing in practice. When calling it, m_directory was always nullptr. I'll actually change this so we still call removeFromDirectory() here and remove the call from MarkedSpace::freeBlock. That way it's a bit more obvious what's going on.
> How about instead adding an enum class WillDeleteBlock { Yes, No } parameter > to removeBlock() that defaults to WillDeleteBlock::No? Then removeBlock() > can call didRemoveFromDirectory if willDeleteBlock == WillDeleteBlock::YES. I mean NO! :P
Created attachment 426252 [details] Patch for landing
Committed r276155 (236646@main): <https://commits.webkit.org/236646@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 426252 [details].
rdar://76779547