| Summary: | [JSC] Introduce JSNonDestructibleProxy for JavaScriptCore.framework's GlobalThis | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||
| Component: | New Bugs | Assignee: | 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
Yusuke Suzuki
2019-02-18 16:11:19 PST
Created attachment 362354 [details]
Patch
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 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 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 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. After discussing with Saam, I think this is a simple approach for now. 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 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. Committed r241769: <https://trac.webkit.org/changeset/241769> |