Bug 192620 – [DFG][FTL] Add NewSymbol

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[DFG][FTL] Add NewSymbol

[Yusuke Suzuki]

Created attachment 357116 [details]
WIP

[Yusuke Suzuki]

Created attachment 357123 [details]
Patch

[Saam Barati]

Comment on attachment 357123 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        NewSymbol code faster. Rather than that, this patch intends to offer SpecSymbol type information into DFG's

Do you think it could be worth inlining the allocation for this in another patch?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:9662
> +    // Known String.

You should assert instead of comment

[Saam Barati]

Comment on attachment 357123 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:14
> +        itself does not perform any type checks. Since `NewSymbol` will not exit except for OOM, we can just use ToString

It’s worth saying this is because ToString performs effects but NewSymbol doesn’t. Also, I don’t think we will ever OOM here, we will just crash

[Yusuke Suzuki]

Comment on attachment 357123 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:9
>> +        NewSymbol code faster. Rather than that, this patch intends to offer SpecSymbol type information into DFG's
> 
> Do you think it could be worth inlining the allocation for this in another patch?

One problem is that every Symbol has an unique WTF::SymbolImpl, which is allocated from bmalloc heap.
While we can allocate Symbol JSCell in the inline code, we still need to call C runtime to allocate this SymbolImpl.
If it still offers a perf win, it would be worth doing since Symbol creation would be more frequent after class private fields come.

>> Source/JavaScriptCore/ChangeLog:14
>> +        itself does not perform any type checks. Since `NewSymbol` will not exit except for OOM, we can just use ToString
> 
> It’s worth saying this is because ToString performs effects but NewSymbol doesn’t. Also, I don’t think we will ever OOM here, we will just crash

Sounds nice. Fixed.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:9662
>> +    // Known String.
> 
> You should assert instead of comment

Nice, fixed.

[Yusuke Suzuki]

Committed r239142: <https://trac.webkit.org/changeset/239142>

[Radar WebKit Bug Importer]

<rdar://problem/46683296>

[Yusuke Suzuki]

(In reply to Saam Barati from comment #3)
> Comment on attachment 357123 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=357123&action=review
> 
> > Source/JavaScriptCore/ChangeLog:9
> > +        NewSymbol code faster. Rather than that, this patch intends to offer SpecSymbol type information into DFG's
> 
> Do you think it could be worth inlining the allocation for this in another
> patch?

After investigating, inlining allocation does not improve Symbol constructor calls much. Actually, the large part of the time is consumed by HashTable, which is used to ensure one-on-one correspondence betweeen Symbol* and WTF::SymbolImpl.

[Saam Barati]

(In reply to Yusuke Suzuki from comment #8)
> (In reply to Saam Barati from comment #3)
> > Comment on attachment 357123 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=357123&action=review
> > 
> > > Source/JavaScriptCore/ChangeLog:9
> > > +        NewSymbol code faster. Rather than that, this patch intends to offer SpecSymbol type information into DFG's
> > 
> > Do you think it could be worth inlining the allocation for this in another
> > patch?
> 
> After investigating, inlining allocation does not improve Symbol constructor
> calls much. Actually, the large part of the time is consumed by HashTable,
> which is used to ensure one-on-one correspondence betweeen Symbol* and
> WTF::SymbolImpl.

Makes sense. Thanks for investigating.

If we ever found a use case where this is super hot, we could inline cache Symbol(), e.g, a program like this:
function foo() { return Symbol(“const string”) }
Where foo is frequently called. We could cache the underlying SymbolImpl

[Yusuke Suzuki]

(In reply to Saam Barati from comment #9)
> 
> Makes sense. Thanks for investigating.
> 
> If we ever found a use case where this is super hot, we could inline cache
> Symbol(), e.g, a program like this:
> function foo() { return Symbol(“const string”) }
> Where foo is frequently called. We could cache the underlying SymbolImpl

The issue is that, everytime foo is called, it generates a new unique Symbol with unique SymbolImpl. So we cannot perform IC here.
One fancy plan is that, embedding SymbolImpl into Symbol*. And Symbol* won't be collected while SymbolImpl's ref count is larger than one (this could be implemented by marking constraints).
But the problem is, one SymbolImpl generated by VM cannot be used by the other VM.
So, if we would like to do this optimization, I think we need per-process VM heap.

[Saam Barati]

Makes sense. I clearly forgot that we have the 1:1 mapping of SymbolImpl to Symbol*