Bug 224677 – Before deleting a MarkedBlock we do not need to clear its m_directory pointer.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

Before deleting a MarkedBlock we do not need to clear its m_directory pointer.

[Keith Miller]

Created attachment 426246 [details]
Patch

[Mark Lam]

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?

[Keith Miller]

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.

[Keith Miller]

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.

[Geoffrey Garen]

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?

[Yusuke Suzuki]

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.

[Keith Miller]

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.

[Geoffrey Garen]

> 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

[Keith Miller]

Created attachment 426252 [details]
Patch for landing

[EWS]

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].

[Geoffrey Garen]

rdar://76779547