Bug 205107 - Put all generated JSCells in WebCore into IsoSubspace
Summary: Put all generated JSCells in WebCore into IsoSubspace
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: 208704
Blocks:
  Show dependency treegraph
 
Reported: 2019-12-11 02:17 PST by Yusuke Suzuki
Modified: 2020-03-07 16:21 PST (History)
18 users (show)

See Also:


Attachments
Patch (14.15 KB, patch)
2020-03-03 23:02 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (275.73 KB, patch)
2020-03-04 15:47 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (275.84 KB, patch)
2020-03-04 16:24 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (281.55 KB, patch)
2020-03-04 16:41 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (284.32 KB, patch)
2020-03-04 17:40 PST, Yusuke Suzuki
saam: 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-12-11 02:17:11 PST
Do it via CodeGeneratorJS.pm
Comment 1 Yusuke Suzuki 2020-03-03 23:02:22 PST
Created attachment 392377 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2020-03-04 15:37:40 PST
<rdar://problem/60060068>
Comment 3 Yusuke Suzuki 2020-03-04 15:47:01 PST
Created attachment 392497 [details]
Patch
Comment 4 Yusuke Suzuki 2020-03-04 16:24:16 PST
Created attachment 392504 [details]
Patch
Comment 5 Yusuke Suzuki 2020-03-04 16:41:38 PST
Created attachment 392507 [details]
Patch
Comment 6 Yusuke Suzuki 2020-03-04 17:40:21 PST
Created attachment 392521 [details]
Patch
Comment 7 Saam Barati 2020-03-04 18:44:49 PST
Comment on attachment 392521 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4679
> +        push(@implContent, "    else\n");
> +        push(@implContent, "        spaces.m_subspaceFor${interfaceName} = makeUnique<IsoSubspace> ISO_SUBSPACE_INIT(vm.heap, vm.cellHeapCellType.get(), ${className});\n");

can we also assert here that the object is not destructible. Maybe even static assert?
Comment 8 Saam Barati 2020-03-04 18:45:02 PST
r=me
Comment 9 Yusuke Suzuki 2020-03-04 23:18:24 PST
Committed r257905: <https://trac.webkit.org/changeset/257905>
Comment 10 Jason Lawrence 2020-03-05 10:02:24 PST
Reverted r257905 for reason:

This commit caused crashes on Mac wk2 Debug.

Committed r257922: <https://trac.webkit.org/changeset/257922>
Comment 11 Yusuke Suzuki 2020-03-05 15:01:27 PST
Committed r257950: <https://trac.webkit.org/changeset/257950>
Comment 12 WebKit Commit Bot 2020-03-06 02:50:20 PST
Re-opened since this is blocked by bug 208704
Comment 13 Yusuke Suzuki 2020-03-06 02:59:55 PST
Comment on attachment 392521 [details]
Patch

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

> Source/WebCore/bindings/js/WebCoreJSClientData.h:72
> +    Vector<JSC::IsoSubspace*> outputConstraintSpaces() { return m_outputConstraintSpaces; }

This is the bug. We should return Vector<JSC::IsoSubspace*>&.
Comment 14 Yusuke Suzuki 2020-03-06 03:10:45 PST
(In reply to Yusuke Suzuki from comment #13)
> Comment on attachment 392521 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=392521&action=review
> 
> > Source/WebCore/bindings/js/WebCoreJSClientData.h:72
> > +    Vector<JSC::IsoSubspace*> outputConstraintSpaces() { return m_outputConstraintSpaces; }
> 
> This is the bug. We should return Vector<JSC::IsoSubspace*>&.

I've ensured this fixes the debug assertion problem.
Comment 15 Yusuke Suzuki 2020-03-06 03:11:47 PST
Committed r257975: <https://trac.webkit.org/changeset/257975>
Comment 16 Fujii Hironori 2020-03-06 17:00:08 PST
Do you want to put WebCoreTestSupport objects into IsoSubspace?
It seems a layer violation that WebCore depends on WebCoreTestSupport.
Comment 17 Yusuke Suzuki 2020-03-07 08:07:24 PST
(In reply to Fujii Hironori from comment #16)
> Do you want to put WebCoreTestSupport objects into IsoSubspace?
> It seems a layer violation that WebCore depends on WebCoreTestSupport.

In Apple ports, all IDL files are preprocessed first, and at this point, we are generating IsoSubspaces.
Comment 18 Yusuke Suzuki 2020-03-07 08:16:59 PST
(In reply to Yusuke Suzuki from comment #17)
> (In reply to Fujii Hironori from comment #16)
> > Do you want to put WebCoreTestSupport objects into IsoSubspace?
> > It seems a layer violation that WebCore depends on WebCoreTestSupport.
> 
> In Apple ports, all IDL files are preprocessed first, and at this point, we
> are generating IsoSubspaces.

Currently, in Apple ports, preprocess-idls.pl phase does not have layering between support ones and WebCore ones. All IDLs are preprocessed at once. So IsoSubspaces are generated at this point. That’s why IsoSubspaces for testing things are also generated here for now.
Comment 19 Fujii Hironori 2020-03-07 16:21:30 PST
It makes WebCore can't build without WebCoreTestSupport's IDL files.
There is no actual problem. It's just a layer matter.