WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
198407
[JSC] InferredValue should not be a JSCell
https://bugs.webkit.org/show_bug.cgi?id=198407
Summary
[JSC] InferredValue should not be a JSCell
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
Details
Formatted Diff
Diff
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
Details
Formatted Diff
Diff
Patch
(68.27 KB, patch)
2019-06-03 23:03 PDT
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
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.
Top of Page
Format For Printing
XML
Clone This Bug