Bug 193993 – [JSC] Shrink size of VM by lazily allocating IsoSubspaces for non-common types

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

[Yusuke Suzuki]

VM has a lot of IsoSubspaces, and each takes 504B.
Some of them are rarely used. We should allocate it lazily.

[Yusuke Suzuki]

Created attachment 360518 [details]
Patch

[Yusuke Suzuki]

Created attachment 360522 [details]
Patch

[EWS Watchlist]

Attachment 360522 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCCallbackFunction.h"
Total errors found: 1 in 62 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 360523 [details]
Patch

[Yusuke Suzuki]

It seems this causes crashes. Investigating...

[EWS Watchlist]

Comment on attachment 360523 [details]
Patch

Attachment 360523 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10945570

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360534 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360523 [details]
Patch

Attachment 360523 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10945839

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360535 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360523 [details]
Patch

Attachment 360523 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10945790

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360537 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360523 [details]
Patch

Attachment 360523 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/10945716

New failing tests:
stress/const-and-with-statement.js.no-cjit-collect-continuously
stress/scope-operation-cache-global-property-before-deleting.js.dfg-eager-no-cjit-validate
stress/const-and-with-statement.js.ftl-eager
stress/scope-operation-cache-global-property-before-deleting.js.ftl-eager-no-cjit
stress/global-lexical-variable-with-statement.js.dfg-eager-no-cjit-validate
stress/class-expression-generates-environment.js.dfg-eager
apiTests

[EWS Watchlist]

Comment on attachment 360523 [details]
Patch

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

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360544 [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

[Yusuke Suzuki]

Created attachment 360547 [details]
Patch

[Yusuke Suzuki]

Created attachment 360550 [details]
Patch

[EWS Watchlist]

Comment on attachment 360550 [details]
Patch

Attachment 360550 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10947926

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360554 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360550 [details]
Patch

Attachment 360550 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10947835

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360555 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360550 [details]
Patch

Attachment 360550 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10948035

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360556 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360550 [details]
Patch

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

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360557 [details]
Archive of layout-test-results from ews204 for win-future

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

[EWS Watchlist]

Comment on attachment 360550 [details]
Patch

Attachment 360550 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10948080

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360558 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Yusuke Suzuki]

Cool! Finally I found that this hits the existing bug!
In VM shutdown, if UnlinkedCodeBlock is destroyed before linked CodeBlock, underlying UnlinkedMetadataTable is already gone when destroying CodeBlock.

[Yusuke Suzuki]

Previously, EvalCodeBlock is visited before visiting UnlinkedCodeBlock. At that case, we first destroy MetadataTable, unlinking it from the UnlinkedMetadataTable, and then, UnlinkedCodeBlock is destroyed, which leads to destruction of UnlinkedMetadataTable. This is fine.
But this dynamic IsoSubspace creation churns the implicit order of BlockDirectories filed in ObjectSpace (MarkedSpace). When VM shutdown happens, and UnlinkedCodeBlock is destroyed before EvalCodeBlock, then UnlinkedMetadataTable is destroyed while it is still linked.

We can test this case easily by the following script with the above patch.

$.agent.start(`
for (var i = 0; i < 10000; ++i) {
    eval('print(' + i + ')');
}
`);
sleepSeconds(10);

[Yusuke Suzuki]

Let's make InferredValue IsoSubspace lazy too.

[Yusuke Suzuki]

Created attachment 361042 [details]
Patch

[Keith Miller]

Comment on attachment 361042 [details]
Patch

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

r- since I think we can do a fair amount of code de-duplication.

Can we forward the SubSpaceAccess to the VM's getter? Currently, both the subspaceFor and the VM's getter methods have to know about concurrent access. If you make the getter on VM be:

Subspace* webAssemblyFunctionSpace(SubspaceAccess mode)
{
    if (m_ webAssemblyFunctionSpace || mode == SubspaceAccess::Concurrent)
        return webAssemblyFunctionSpace.get();
    return ensureWebAssemblyFunctionSpace();
}

Then all the various subspaceFor can be:

template<typename CellType, SubspaceAccess mode>
static Subspace* subspaceFor(VM& vm)
{
    return vm. webAssemblyFunctionSpace(mode);
}

Which makes them a lot simpler and doesn't make the VM code any more complicated since you'll only have one method instead of two.

> Source/JavaScriptCore/runtime/JSCell.h:308
> +inline auto subspaceForConcurrently(VM& vm)
> +{
> +    return Type::template subspaceFor<Type, SubspaceAccess::Concurrently>(vm);

Why not have:

template<typename Type>
inline Allocator allocatorForNonVirtualConcurrently(VM& vm, size_t allocationSize, AllocatorForMode mode)
{
    if (auto subspace = Type::template subspaceFor<Type, SubspaceAccess::Concurrently>(vm))
        return subspace->allocatorForNonVirtual(allocationSize, mode);
   return { };
}

Then you wouldn't have to duplicate nearly as much code in the JITs.

> Source/JavaScriptCore/runtime/VM.cpp:1228
> +IsoSubspace& VM::ensureBoundFunctionSpaceSlow()
> +{
> +    ASSERT(!boundFunctionSpace);
> +    auto space = std::make_unique<IsoSubspace> ISO_SUBSPACE_INIT(heap, cellHeapCellType.get(), JSBoundFunction);
> +    WTF::storeStoreFence();
> +    boundFunctionSpace = WTFMove(space);
> +    return *boundFunctionSpace;
> +}

Nit: Can we make a template function or worse-case a macro that does this? This is a lot of duplicated code. :(

[Yusuke Suzuki]

Comment on attachment 361042 [details]
Patch

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

Thanks. Make sense, we should reduce the duplication by using macro.

>> Source/JavaScriptCore/runtime/JSCell.h:308
>> +    return Type::template subspaceFor<Type, SubspaceAccess::Concurrently>(vm);
> 
> Why not have:
> 
> template<typename Type>
> inline Allocator allocatorForNonVirtualConcurrently(VM& vm, size_t allocationSize, AllocatorForMode mode)
> {
>     if (auto subspace = Type::template subspaceFor<Type, SubspaceAccess::Concurrently>(vm))
>         return subspace->allocatorForNonVirtual(allocationSize, mode);
>    return { };
> }
> 
> Then you wouldn't have to duplicate nearly as much code in the JITs.

Make sense. I've changed them to allocatorForNonVirtualConcurrently. But still some uses Subspace* (not Allocator). For those cases, I use subspaceForConcurrently.

>> Source/JavaScriptCore/runtime/VM.cpp:1228
>> +}
> 
> Nit: Can we make a template function or worse-case a macro that does this? This is a lot of duplicated code. :(

Nice. Since this depends on the name like "boundFunctionSpace", I think macro is the easiest and cleanest solution here. Changed.

[Yusuke Suzuki]

Created attachment 361096 [details]
Patch

[Keith Miller]

Comment on attachment 361096 [details]
Patch

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

r=me with some nits. Looks good!

> Source/JavaScriptCore/ChangeLog:31
> +        can run this IsoSubspace code, the collector is never EndPhase. So this is safe.

Is there anyway to assert that the GC isn't in EndPhase when creating an IsoSubspace? Even if it's racy that might still be helpful. I would only worry about this if it's like a one line change though.

> Source/JavaScriptCore/runtime/VM.cpp:1248
> +

Can we undef DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER_SLOW here?

> Source/JavaScriptCore/runtime/VM.cpp:1263
> +

Ditto on undef.

> Source/JavaScriptCore/runtime/VM.h:406
>      

Ditto on undef.

> Source/JavaScriptCore/runtime/VM.h:458
> +    DYNAMIC_SPACE_AND_SET_DEFINE_MEMBER(moduleProgramExecutableSpace)

Ditto on undef.

[Yusuke Suzuki]

Comment on attachment 361096 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ChangeLog:31
>> +        can run this IsoSubspace code, the collector is never EndPhase. So this is safe.
> 
> Is there anyway to assert that the GC isn't in EndPhase when creating an IsoSubspace? Even if it's racy that might still be helpful. I would only worry about this if it's like a one line change though.

Nice, I'm now checking this with `ASSERT(!mayBeGCThread() || m_heap->worldIsStopped());` now to ensure the above thing!

>> Source/JavaScriptCore/runtime/VM.cpp:1248
>> +
> 
> Can we undef DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER_SLOW here?

Done.

>> Source/JavaScriptCore/runtime/VM.cpp:1263
>> +
> 
> Ditto on undef.

Done.

>> Source/JavaScriptCore/runtime/VM.h:406
>>      
> 
> Ditto on undef.

Done.

>> Source/JavaScriptCore/runtime/VM.h:458
>> +    DYNAMIC_SPACE_AND_SET_DEFINE_MEMBER(moduleProgramExecutableSpace)
> 
> Ditto on undef.

Done.

[Yusuke Suzuki]

Committed r240965: <https://trac.webkit.org/changeset/240965>

[Radar WebKit Bug Importer]

<rdar://problem/47810714>