Bug 194799 – [JSC] Introduce JSNonDestructibleProxy for JavaScriptCore.framework's GlobalThis

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Introduce JSNonDestructibleProxy for JavaScriptCore.framework's GlobalThis

[Yusuke Suzuki]

Created attachment 362354 [details]
Patch

[Saam Barati]

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.

[Saam Barati]

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

[Yusuke Suzuki]

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/

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

After discussing with Saam, I think this is a simple approach for now.

[Saam Barati]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r241769: <https://trac.webkit.org/changeset/241769>

[Radar WebKit Bug Importer]

<rdar://problem/48209822>