Bug 198407

Summary: [JSC] InferredValue should not be a JSCell
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: 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 Flags
Patch
none
Archive of layout-test-results from ews211 for win-future
none
Patch
none
Patch none

Yusuke Suzuki
Reported 2019-05-30 20:24:19 PDT
All FunctionExecutable has this. And we do not have interesting policy like CodeBlock edge. We should embed this feature directly to FunctionExecutable so that we can save memory consumed by InferredValue.
Attachments
Patch (66.90 KB, patch)
2019-05-31 23:56 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews211 for win-future (13.55 MB, application/zip)
2019-06-01 04:35 PDT, EWS Watchlist 		no flags
Details
Patch (66.94 KB, patch)
2019-06-01 16:14 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (68.27 KB, patch)
2019-06-03 23:03 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1 2019-05-31 21:35:12 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.
Radar WebKit Bug Importer
Comment 2 2019-05-31 23:04:44 PDT
<rdar://problem/51326016>
Yusuke Suzuki
Comment 3 2019-05-31 23:56:14 PDT
Created attachment 371109 [details] Patch
EWS Watchlist
Comment 4 2019-06-01 04:35:22 PDT
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
EWS Watchlist
Comment 5 2019-06-01 04:35:25 PDT
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
Yusuke Suzuki
Comment 6 2019-06-01 16:14:35 PDT
Created attachment 371128 [details] Patch
Yusuke Suzuki
Comment 7 2019-06-03 23:03:32 PDT
Created attachment 371250 [details] Patch
Yusuke Suzuki
Comment 8 2019-06-04 10:56:47 PDT
Comment on attachment 371250 [details] Patch Thanks, landing.
WebKit Commit Bot
Comment 9 2019-06-04 11:28:06 PDT
Comment on attachment 371250 [details] Patch Clearing flags on attachment: 371250 Committed r246073: <https://trac.webkit.org/changeset/246073>
WebKit Commit Bot
Comment 10 2019-06-04 11:28:08 PDT
All reviewed patches have been landed. Closing bug.
Saam Barati
Comment 11 2019-06-04 18:46:16 PDT
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
Yusuke Suzuki
Comment 12 2019-06-04 19:13:39 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.