Bug 194799

Summary: [JSC] Introduce JSNonDestructibleProxy for JavaScriptCore.framework's GlobalThis
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch saam: review+

Yusuke Suzuki
Reported 2019-02-18 16:11:19 PST
[JSC] Introduce JSNonDestructibleProxy for JavaScriptCore.framework's GlobalThis
Attachments
Patch (15.95 KB, patch)
2019-02-18 16:14 PST, Yusuke Suzuki
saam: review+
Yusuke Suzuki
Comment 1 2019-02-18 16:14:30 PST
Saam Barati
Comment 2 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.
Saam Barati
Comment 3 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
Yusuke Suzuki
Comment 4 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/
Yusuke Suzuki
Comment 5 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.
Yusuke Suzuki
Comment 6 2019-02-19 01:19:26 PST
After discussing with Saam, I think this is a simple approach for now.
Saam Barati
Comment 7 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.
Yusuke Suzuki
Comment 8 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.
Yusuke Suzuki
Comment 9 2019-02-19 12:15:02 PST
Radar WebKit Bug Importer
Comment 10 2019-02-19 12:16:00 PST
Note You need to log in before you can comment on or make changes to this bug.