Bug 194799 - [JSC] Introduce JSNonDestructibleProxy for JavaScriptCore.framework's GlobalThis
Summary: [JSC] Introduce JSNonDestructibleProxy for JavaScriptCore.framework's GlobalThis
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-18 16:11 PST by Yusuke Suzuki
Modified: 2019-02-19 12:16 PST (History)
2 users (show)

See Also:


Attachments
Patch (15.95 KB, patch)
2019-02-18 16:14 PST, Yusuke Suzuki
sbarati: 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-02-18 16:11:19 PST
[JSC] Introduce JSNonDestructibleProxy for JavaScriptCore.framework's GlobalThis
Comment 1 Yusuke Suzuki 2019-02-18 16:14:30 PST
Created attachment 362354 [details]
Patch
Comment 2 Saam Barati 2019-02-18 16:20:35 PST
Comment on attachment 362354 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:12
> +        This patch adds JSNonDestructibleProxy, which is not destructible JSProxy. While it inherits JSDestructibleObject,

not destructible => non-destructible

> Source/JavaScriptCore/ChangeLog:13
> +        we can make the subclass still non-destructible thanks to Subspace mechanism. This drops one more low-usage MarkedBlock.

ooph. Not sure I'm a fan of that.
Comment 3 Saam Barati 2019-02-18 16:20:55 PST
Comment on attachment 362354 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:13
>> +        we can make the subclass still non-destructible thanks to Subspace mechanism. This drops one more low-usage MarkedBlock.
> 
> ooph. Not sure I'm a fan of that.

Do we do this anywhere else? Feels super hacky
Comment 4 Yusuke Suzuki 2019-02-18 16:27:26 PST
Comment on attachment 362354 [details]
Patch

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

>>> Source/JavaScriptCore/ChangeLog:13
>>> +        we can make the subclass still non-destructible thanks to Subspace mechanism. This drops one more low-usage MarkedBlock.
>> 
>> ooph. Not sure I'm a fan of that.
> 
> Do we do this anywhere else? Feels super hacky

Opposite one can be found, like, JSGlobalLexicalEnvironment. It does not inherit JSDestructibleObject, but it want to make itself destructible. Currently, they has separate CompleteSubspace just for that (I don't like this because it is too costly).
Currently, only two JSProxies are created in simple JSC memory profile benchmarks, and it allocates 16KB, which should be dropped because this is not necessary essentially.

Another idea is that not allocating JSProxy for JavaScriptCore.framework. We just use JSGlobalObject for |this| / |globalThis| instead of putting JSProxy objects for that. This is OK since the spec says nothing about it[1].
This patch is a bit hacky because it takes the way not changing the current behavior of JavaScriptCore.framework. But if it is allowed, we can remove JSProxy instead. Anyway, I think we should not allocate additional MarkedBlock only for that.
[1]: https://tc39.github.io/proposal-global/
Comment 5 Yusuke Suzuki 2019-02-18 16:40:47 PST
Comment on attachment 362354 [details]
Patch

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

>>>> Source/JavaScriptCore/ChangeLog:13
>>>> +        we can make the subclass still non-destructible thanks to Subspace mechanism. This drops one more low-usage MarkedBlock.
>>> 
>>> ooph. Not sure I'm a fan of that.
>> 
>> Do we do this anywhere else? Feels super hacky
> 
> Opposite one can be found, like, JSGlobalLexicalEnvironment. It does not inherit JSDestructibleObject, but it want to make itself destructible. Currently, they has separate CompleteSubspace just for that (I don't like this because it is too costly).
> Currently, only two JSProxies are created in simple JSC memory profile benchmarks, and it allocates 16KB, which should be dropped because this is not necessary essentially.
> 
> Another idea is that not allocating JSProxy for JavaScriptCore.framework. We just use JSGlobalObject for |this| / |globalThis| instead of putting JSProxy objects for that. This is OK since the spec says nothing about it[1].
> This patch is a bit hacky because it takes the way not changing the current behavior of JavaScriptCore.framework. But if it is allowed, we can remove JSProxy instead. Anyway, I think we should not allocate additional MarkedBlock only for that.
> [1]: https://tc39.github.io/proposal-global/

I quickly grepped JSC source with `public JSDestructibleObject` thing and it seems like this JSProxy thing is only the objects which (1) has destructor and (2) has the same size class in JSC.framework objects :(
So, this MarkedBlock is just used for this two JSProxy.
Comment 6 Yusuke Suzuki 2019-02-19 01:19:26 PST
After discussing with Saam, I think this is a simple approach for now.
Comment 7 Saam Barati 2019-02-19 11:23:55 PST
Comment on attachment 362354 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSNonDestructibleProxy.cpp:2
> + * Copyright (C) 2011-2012, 2016-2017 Apple Inc. All rights reserved.

2019

> Source/JavaScriptCore/runtime/JSNonDestructibleProxy.h:41
> +        return JSNonFinalObject::subspaceFor<CellType, mode>(vm);

it's worth a comment here IMO given how this is a bit weird.
Comment 8 Yusuke Suzuki 2019-02-19 11:51:27 PST
Comment on attachment 362354 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSNonDestructibleProxy.cpp:2
>> + * Copyright (C) 2011-2012, 2016-2017 Apple Inc. All rights reserved.
> 
> 2019

Oh! Thanks.

>> Source/JavaScriptCore/runtime/JSNonDestructibleProxy.h:41
>> +        return JSNonFinalObject::subspaceFor<CellType, mode>(vm);
> 
> it's worth a comment here IMO given how this is a bit weird.

Right. I'll do that now.
Comment 9 Yusuke Suzuki 2019-02-19 12:15:02 PST
Committed r241769: <https://trac.webkit.org/changeset/241769>
Comment 10 Radar WebKit Bug Importer 2019-02-19 12:16:00 PST
<rdar://problem/48209822>