Bug 193993 - [JSC] Shrink size of VM by lazily allocating IsoSubspaces for non-common types
Summary: [JSC] Shrink size of VM by lazily allocating IsoSubspaces for non-common types
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on: 194031 194084
Blocks: 193606
  Show dependency treegraph
 
Reported: 2019-01-29 15:51 PST by Yusuke Suzuki
Modified: 2019-02-04 22:33 PST (History)
8 users (show)

See Also:


Attachments
Patch (60.46 KB, patch)
2019-01-29 15:58 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (83.89 KB, patch)
2019-01-29 16:34 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (83.86 KB, patch)
2019-01-29 16:36 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (3.19 MB, application/zip)
2019-01-29 18:12 PST, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-highsierra (2.87 MB, application/zip)
2019-01-29 18:26 PST, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (23.44 MB, application/zip)
2019-01-29 19:00 PST, Build Bot
no flags Details
Archive of layout-test-results from ews200 for win-future (11.33 MB, application/zip)
2019-01-29 20:26 PST, Build Bot
no flags Details
Patch (87.61 KB, patch)
2019-01-29 20:45 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (85.30 KB, patch)
2019-01-29 21:18 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (2.73 MB, application/zip)
2019-01-29 22:31 PST, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (2.53 MB, application/zip)
2019-01-29 22:42 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.85 MB, application/zip)
2019-01-29 22:49 PST, Build Bot
no flags Details
Archive of layout-test-results from ews204 for win-future (11.61 MB, application/zip)
2019-01-29 23:18 PST, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.68 MB, application/zip)
2019-01-29 23:24 PST, Build Bot
no flags Details
Patch (80.58 KB, patch)
2019-02-03 23:33 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (85.08 KB, patch)
2019-02-04 13:55 PST, Yusuke Suzuki
keith_miller: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-01-29 15:51:00 PST
VM has a lot of IsoSubspaces, and each takes 504B.
Some of them are rarely used. We should allocate it lazily.
Comment 1 Yusuke Suzuki 2019-01-29 15:58:58 PST
Created attachment 360518 [details]
Patch
Comment 2 Yusuke Suzuki 2019-01-29 16:34:08 PST
Created attachment 360522 [details]
Patch
Comment 3 Build Bot 2019-01-29 16:35:52 PST
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.
Comment 4 Yusuke Suzuki 2019-01-29 16:36:08 PST
Created attachment 360523 [details]
Patch
Comment 5 Yusuke Suzuki 2019-01-29 17:58:58 PST
It seems this causes crashes. Investigating...
Comment 6 Build Bot 2019-01-29 18:12:43 PST
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.
Comment 7 Build Bot 2019-01-29 18:12:45 PST
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
Comment 8 Build Bot 2019-01-29 18:26:21 PST
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.
Comment 9 Build Bot 2019-01-29 18:26:23 PST
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
Comment 10 Build Bot 2019-01-29 19:00:31 PST
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.
Comment 11 Build Bot 2019-01-29 19:00:37 PST
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
Comment 12 Build Bot 2019-01-29 19:25:50 PST
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
Comment 13 Build Bot 2019-01-29 20:26:41 PST
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.
Comment 14 Build Bot 2019-01-29 20:26:51 PST
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
Comment 15 Yusuke Suzuki 2019-01-29 20:45:52 PST
Created attachment 360547 [details]
Patch
Comment 16 Yusuke Suzuki 2019-01-29 21:18:14 PST
Created attachment 360550 [details]
Patch
Comment 17 Build Bot 2019-01-29 22:31:06 PST
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.
Comment 18 Build Bot 2019-01-29 22:31:08 PST
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
Comment 19 Build Bot 2019-01-29 22:42:14 PST
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.
Comment 20 Build Bot 2019-01-29 22:42:16 PST
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
Comment 21 Build Bot 2019-01-29 22:49:15 PST
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.
Comment 22 Build Bot 2019-01-29 22:49:17 PST
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
Comment 23 Build Bot 2019-01-29 23:17:52 PST
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.
Comment 24 Build Bot 2019-01-29 23:18:03 PST
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
Comment 25 Build Bot 2019-01-29 23:24:02 PST
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.
Comment 26 Build Bot 2019-01-29 23:24:04 PST
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
Comment 27 Yusuke Suzuki 2019-01-30 00:48:50 PST
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.
Comment 28 Yusuke Suzuki 2019-01-30 01:04:16 PST
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);
Comment 29 Yusuke Suzuki 2019-01-31 00:23:15 PST
Let's make InferredValue IsoSubspace lazy too.
Comment 30 Yusuke Suzuki 2019-02-03 23:33:05 PST
Created attachment 361042 [details]
Patch
Comment 31 Keith Miller 2019-02-04 09:58:05 PST
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. :(
Comment 32 Yusuke Suzuki 2019-02-04 13:52:36 PST
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.
Comment 33 Yusuke Suzuki 2019-02-04 13:55:38 PST
Created attachment 361096 [details]
Patch
Comment 34 Keith Miller 2019-02-04 19:15:18 PST
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.
Comment 35 Yusuke Suzuki 2019-02-04 20:34:32 PST
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.
Comment 36 Yusuke Suzuki 2019-02-04 22:32:13 PST
Committed r240965: <https://trac.webkit.org/changeset/240965>
Comment 37 Radar WebKit Bug Importer 2019-02-04 22:33:45 PST
<rdar://problem/47810714>