Summary: | Before deleting a MarkedBlock we do not need to clear its m_directory pointer. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keith Miller <keith_miller> | ||||||
Component: | New Bugs | Assignee: | Keith Miller <keith_miller> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ews-watchlist, ggaren, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Keith Miller
2021-04-16 10:06:20 PDT
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]. |