Bug 194939

Summary: [JSC] SmallStringsStorage is unnecessary
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, ggaren, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch mark.lam: review+

Description Yusuke Suzuki 2019-02-22 01:33:15 PST
[JSC] SmallStringsStorage is unnecessary
Comment 1 Yusuke Suzuki 2019-02-22 01:54:23 PST
Created attachment 362709 [details]
Patch
Comment 2 Yusuke Suzuki 2019-02-22 02:00:33 PST
Comment on attachment 362709 [details]
Patch

Oops, fixing build issue.
Comment 3 Yusuke Suzuki 2019-02-22 02:05:50 PST
Created attachment 362710 [details]
Patch
Comment 4 Yusuke Suzuki 2019-02-22 02:10:29 PST
Created attachment 362711 [details]
Patch
Comment 5 Mark Lam 2019-02-22 09:52:12 PST
Comment on attachment 362711 [details]
Patch

r=me
Comment 6 Geoffrey Garen 2019-02-22 10:37:51 PST
Comment on attachment 362711 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362711&action=review

> Source/JavaScriptCore/runtime/SmallStrings.h:76
> +    void setCanAccessHeap(bool canAccessHeap) { m_canAccessHeap = canAccessHeap; }

I would call this isInitialized / setIsInitialized.

> Source/JavaScriptCore/runtime/VM.cpp:543
> +    smallStrings.setCanAccessHeap(false);

Is there code that tries to access the SmallStrings cache inside lastChanceToFinalize? If so, that feels like a bug.
Comment 7 Yusuke Suzuki 2019-02-22 10:52:24 PST
Comment on attachment 362711 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362711&action=review

>> Source/JavaScriptCore/runtime/SmallStrings.h:76
>> +    void setCanAccessHeap(bool canAccessHeap) { m_canAccessHeap = canAccessHeap; }
> 
> I would call this isInitialized / setIsInitialized.

OK, changed.

>> Source/JavaScriptCore/runtime/VM.cpp:543
>> +    smallStrings.setCanAccessHeap(false);
> 
> Is there code that tries to access the SmallStrings cache inside lastChanceToFinalize? If so, that feels like a bug.

I don't see the actual code is now using it. But I think touching smallStrings after VM Heap is destroyed can be possible.
This is because Identifier creation touches vm.smallStrings to get single character AtomicStringImpl. I think it is a possible story that we create Identifier after VM Heap is destroyed, because Identifier seems unrelated to VM GC Heap.
So I take safer design here.
Comment 8 Yusuke Suzuki 2019-02-22 11:04:46 PST
Committed r241954: <https://trac.webkit.org/changeset/241954>
Comment 9 Radar WebKit Bug Importer 2019-02-22 11:11:03 PST
<rdar://problem/48317238>
Comment 10 Yusuke Suzuki 2019-02-22 11:15:48 PST
Committed r241955: <https://trac.webkit.org/changeset/241955>