Bug 186877 - Use IsoCellSets to track Executables with clearable code.
Summary: Use IsoCellSets to track Executables with clearable code.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-06-20 23:32 PDT by Mark Lam
Modified: 2018-06-21 14:21 PDT (History)
9 users (show)

See Also:


Attachments
proposed patch. (30.27 KB, patch)
2018-06-21 00:20 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews200 for win-future (12.94 MB, application/zip)
2018-06-21 05:03 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2018-06-20 23:32:56 PDT
When deleting all code, this allows us to avoid touching pages of memory for Executables with no code to clear.

<rdar://problem/40910419>
Comment 1 Mark Lam 2018-06-21 00:20:44 PDT
Created attachment 343220 [details]
proposed patch.
Comment 2 EWS Watchlist 2018-06-21 05:03:00 PDT
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
Comment 3 EWS Watchlist 2018-06-21 05:03:10 PDT
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
Comment 4 WebKit Commit Bot 2018-06-21 08:09:46 PDT
Comment on attachment 343220 [details]
proposed patch.

Clearing flags on attachment: 343220

Committed r233039: <https://trac.webkit.org/changeset/233039>
Comment 5 WebKit Commit Bot 2018-06-21 08:09:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Saam Barati 2018-06-21 11:54:37 PDT
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.
Comment 7 Filip Pizlo 2018-06-21 12:04:10 PDT
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.
Comment 8 Saam Barati 2018-06-21 14:20:34 PDT
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
Comment 9 Saam Barati 2018-06-21 14:21:52 PDT
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.