Bug 198407 - [JSC] InferredValue should not be a JSCell
Summary: [JSC] InferredValue should not be a JSCell
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks: 198592
  Show dependency treegraph
 
Reported: 2019-05-30 20:24 PDT by Yusuke Suzuki
Modified: 2019-06-05 18:46 PDT (History)
8 users (show)

See Also:


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, Build Bot
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

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 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.
Comment 1 Yusuke Suzuki 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.
Comment 2 Radar WebKit Bug Importer 2019-05-31 23:04:44 PDT
<rdar://problem/51326016>
Comment 3 Yusuke Suzuki 2019-05-31 23:56:14 PDT
Created attachment 371109 [details]
Patch
Comment 4 Build Bot 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
Comment 5 Build Bot 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
Comment 6 Yusuke Suzuki 2019-06-01 16:14:35 PDT
Created attachment 371128 [details]
Patch
Comment 7 Yusuke Suzuki 2019-06-03 23:03:32 PDT
Created attachment 371250 [details]
Patch
Comment 8 Yusuke Suzuki 2019-06-04 10:56:47 PDT
Comment on attachment 371250 [details]
Patch

Thanks, landing.
Comment 9 WebKit Commit Bot 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>
Comment 10 WebKit Commit Bot 2019-06-04 11:28:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Saam Barati 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
Comment 12 Yusuke Suzuki 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.