When deleting all code, this allows us to avoid touching pages of memory for Executables with no code to clear. <rdar://problem/40910419>
Created attachment 343220 [details] proposed patch.
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
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 on attachment 343220 [details] proposed patch. Clearing flags on attachment: 343220 Committed r233039: <https://trac.webkit.org/changeset/233039>
All reviewed patches have been landed. Closing bug.
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 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 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 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.