Bug 186877 – Use IsoCellSets to track Executables with clearable code.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

When deleting all code, this allows us to avoid touching pages of memory for Executables with no code to clear.

<rdar://problem/40910419>

[Mark Lam]

Created attachment 343220 [details]
proposed patch.

[EWS Watchlist]

Comment on attachment 343220 [details]
proposed patch.

Attachment 343220 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8276079

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

[EWS Watchlist]

Created attachment 343236 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[WebKit Commit Bot]

Comment on attachment 343220 [details]
proposed patch.

Clearing flags on attachment: 343220

Committed r233039: <https://trac.webkit.org/changeset/233039>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

Comment on attachment 343220 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=343220&action=review

> Source/JavaScriptCore/runtime/VM.h:430
> +    struct ScriptExecutableSpaceAndSet {

This struct is identical to the one below. It's also identical to SpaceAndFinalizerSet. Why not just give all three a more abstract name? It's weird to declare this three times.

[Filip Pizlo]

Comment on attachment 343220 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=343220&action=review

>> Source/JavaScriptCore/runtime/VM.h:430
>> +    struct ScriptExecutableSpaceAndSet {
> 
> This struct is identical to the one below. It's also identical to SpaceAndFinalizerSet. Why not just give all three a more abstract name? It's weird to declare this three times.

Yeah, just rename one of them to SpaceAndSet and delete the other two.

OTOH, it's nice that these structs make it explicit what the IsoCellSet is for.  Because of that, I don't feel strongly either way.

One compromise might be to abstract the "*bitwise_cast<IsoCellSet*>(bitwise_cast<char*>(&space) - OBJECT_OFFSETOF(ScriptExecutableSpaceAndSet, space) + OBJECT_OFFSETOF(ScriptExecutableSpaceAndSet, clearableCodeSet))" thing.  If that had a macro then the three structs wouldn't bother me.

[Saam Barati]

Comment on attachment 343220 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=343220&action=review

> Source/JavaScriptCore/heap/Heap.cpp:900
> +            HeapIterationScope heapIterationScope(*this);

I think you can put this above the forEachScriptExecutableSpace

[Saam Barati]

Comment on attachment 343220 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=343220&action=review

>>> Source/JavaScriptCore/runtime/VM.h:430
>>> +    struct ScriptExecutableSpaceAndSet {
>> 
>> This struct is identical to the one below. It's also identical to SpaceAndFinalizerSet. Why not just give all three a more abstract name? It's weird to declare this three times.
> 
> Yeah, just rename one of them to SpaceAndSet and delete the other two.
> 
> OTOH, it's nice that these structs make it explicit what the IsoCellSet is for.  Because of that, I don't feel strongly either way.
> 
> One compromise might be to abstract the "*bitwise_cast<IsoCellSet*>(bitwise_cast<char*>(&space) - OBJECT_OFFSETOF(ScriptExecutableSpaceAndSet, space) + OBJECT_OFFSETOF(ScriptExecutableSpaceAndSet, clearableCodeSet))" thing.  If that had a macro then the three structs wouldn't bother me.

Right. I'm not usually not against some duplication in favor of naming, but I'm not sure this is a strong enough use case. That said, this isn't such a big deal either way. It just feels slightly weird because we usually avoid this kind of duplication.