Summary: | [JSC] InferredValue should not be a JSCell | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||||
Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, ews-watchlist, fpizlo, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 198592 | ||||||||||||
Attachments: |
|
Description
Yusuke Suzuki
2019-05-30 20:24:19 PDT
We can create very hacky & fany InferredValue<> thing, and then, we can remove InferredValue completely, and gets 2-3 MB memory reduction in Gmail. Created attachment 371109 [details]
Patch
Comment on attachment 371109 [details] Patch Attachment 371109 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12347214 New failing tests: storage/indexeddb/modern/get-keyrange.html Created attachment 371112 [details]
Archive of layout-test-results from ews211 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 371128 [details]
Patch
Created attachment 371250 [details]
Patch
Comment on attachment 371250 [details]
Patch
Thanks, landing.
Comment on attachment 371250 [details] Patch Clearing flags on attachment: 371250 Committed r246073: <https://trac.webkit.org/changeset/246073> All reviewed patches have been landed. Closing bug. Comment on attachment 371250 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371250&action=review > Source/JavaScriptCore/ChangeLog:16 > + InferredValue<> needs validation in GC finalize phase. So this patch also makes SymbolTable Iso-allocated. I don’t understand why that’s needed? > Source/JavaScriptCore/runtime/InferredValue.h:257 > + m_data = (bitwise_cast<uintptr_t>(value) & ValueMask) | encodeState(IsWatched); Nit: Worth asserting the bits flowing in stay the same when masked Comment on attachment 371250 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371250&action=review >> Source/JavaScriptCore/ChangeLog:16 >> + InferredValue<> needs validation in GC finalize phase. So this patch also makes SymbolTable Iso-allocated. > > I don’t understand why that’s needed? Old InferredValue does not mark the child cell. And every time GC finalizes, InferredValue checks the liveness of the child, and if it is live, keeps holding it, otherwise, invalidates itself and clear the child. So this is something like a Weak<> + Watchpoint. We want to keep this semantics in new InferredValue<>. So what we are doing is, we don't visit InferredValue's value (as the same to old InferredValue), and then each finalizer of the owner classes validates the contained cell, as InferedValue did before. Then, we want to execute unconditional finalizer of the owner cell of InferredValue<>, which is FunctionExecutable and SymbolTable. Previously, this was InferredValue cell. >> Source/JavaScriptCore/runtime/InferredValue.h:257 >> + m_data = (bitwise_cast<uintptr_t>(value) & ValueMask) | encodeState(IsWatched); > > Nit: Worth asserting the bits flowing in stay the same when masked Sounds nice, I'll do it in a follow-up patch. |