Bug 201673 – [JSC] Add StringCodePointAt intrinsic

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

JetStream2/UniPoker uses String#codePointAt much.

Sampling rate: 1000.000000 microseconds
Top functions as <numSamples  'functionName:sourceID'>
   285    '[Symbol.match]:3'
   176    'shift:-1'
   140    'codePointAt:-1'
   111    'join:-1'
    77    'shuffle:4'
    77    'merge:3'
    67    'sort:3'
    50    'mergeSort:3'
    44    'score:4'
    40    'arrayIteratorValueNext:3'
    35    'cardRank:4'
    35    ':5'


Sampling rate: 1000.000000 microseconds
Hottest bytecodes as <numSamples   'functionName#hash:JITType:bytecodeIndex'>
   183    '[Symbol.match]#B7PWYa:Baseline:82 <-- score#Duc7t2:FTL:163'
   176    'shift#<nil>:None:<nil>'
   140    'codePointAt#<nil>:None:<nil>'
   111    'join#<nil>:None:<nil>'
    47    '[Symbol.match]#B7PWYa:Baseline:82 <-- score#Duc7t2:FTL:127'
    41    'shuffle#A4Tfzo:FTL:100'
    30    '[Symbol.match]#B7PWYa:Baseline:82 <-- score#Duc7t2:FTL:91'
    23    'mergeSort#DOiQtk:FTL:22'
    21    'indexOf#<nil>:None:<nil>'
    20    '[Symbol.match]#B7PWYa:Baseline:82 <-- score#Duc7t2:FTL:715'
    18    'clear#CPW4SA:Baseline:13 <-- playHands#DltzVB:FTL:111'
    17    'sort#C0wzoE:FTL:10'
    14    'mergeSort#DOiQtk:FTL:96'
    14    'cardRank#B8GTs9:Baseline:21 <-- merge#AVbsbl:FTL:151'
    12    'merge#AVbsbl:FTL:0'
    11    'merge#AVbsbl:FTL:185'
    11    'shuffle#A4Tfzo:FTL:113'
    10    'newDeck#B6KIhS:Baseline:43 <-- shuffle#A4Tfzo:FTL:18'
     9    'shuffle#A4Tfzo:FTL:88'
     8    'cardRank#B8GTs9:Baseline:13 <-- merge#AVbsbl:FTL:151'
     8    'sort#C0wzoE:FTL:114'
     8    'score#Duc7t2:FTL:682'
     8    'sort#C0wzoE:FTL:66'
     7    'merge#AVbsbl:FTL:212'
     6    '#EMWXIv:FTL:0'
     6    'score#Duc7t2:FTL:1555'
     6    'merge#AVbsbl:FTL:95'
     6    'score#Duc7t2:FTL:32'
     5    'merge#AVbsbl:FTL:224'
     5    'sort#C0wzoE:FTL:126'
     5    '#EMWXIv:FTL:296'
     5    'merge#AVbsbl:FTL:151'
     5    'sort#C0wzoE:FTL:42'
     5    'mergeSort#DOiQtk:FTL:16'
     5    'arrayIteratorValueNext#ANxshv:Baseline:16 <-- score#Duc7t2:FTL:2068'
     5    'merge#AVbsbl:FTL:46'
     5    'merge#AVbsbl:FTL:166'
     5    'sort#C0wzoE:FTL:78'
     4    'sort#C0wzoE:FTL:54'
     4    'shuffle#A4Tfzo:FTL:131'

[Radar WebKit Bug Importer]

<rdar://problem/55246768>

[Yusuke Suzuki]

Ensured that this offers more than 10% Score improvement in JetStream2/UniPoker.

[Yusuke Suzuki]

Created attachment 378538 [details]
Patch

[Yusuke Suzuki]

Created attachment 378539 [details]
Patch

[Yusuke Suzuki]

Created attachment 378546 [details]
Patch

[EWS Watchlist]

Comment on attachment 378546 [details]
Patch

Attachment 378546 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13022548

New failing tests:
stress/string-char-code-at-constant-index-out-of-range.js.no-llint
stress/string-char-code-at-constant-index-out-of-range.js.ftl-no-cjit-validate-sampling-profiler
stress/string-char-code-at-constant-index-out-of-range.js.bytecode-cache
stress/string-char-code-at-constant-index-out-of-range.js.ftl-no-cjit-b3o0
stress/string-char-code-at-constant-index-out-of-range.js.no-cjit-validate-phases
stress/string-char-code-at-constant-index-out-of-range.js.ftl-no-cjit-small-pool
stress/string-char-code-at-constant-index-out-of-range.js.dfg-eager-no-cjit-validate
stress/string-char-code-at-constant-index-out-of-range.js.ftl-no-cjit-no-put-stack-validate
stress/string-char-code-at-constant-index-out-of-range.js.ftl-no-cjit-no-inline-validate
stress/string-char-code-at-constant-index-out-of-range.js.no-cjit-collect-continuously
stress/string-char-code-at-constant-index-out-of-range.js.ftl-eager-no-cjit
stress/string-char-code-at-constant-index-out-of-range.js.no-ftl
stress/string-char-code-at-constant-index-out-of-range.js.ftl-eager-no-cjit-b3o1
stress/string-char-code-at-constant-index-out-of-range.js.default
stress/string-char-code-at-constant-index-out-of-range.js.dfg-eager
stress/string-char-code-at-constant-index-out-of-range.js.eager-jettison-no-cjit
stress/string-char-code-at-constant-index-out-of-range.js.mini-mode
stress/string-char-code-at-constant-index-out-of-range.js.ftl-eager

[Michael Saboff]

Comment on attachment 378546 [details]
Patch

New test stress/string-char-code-at-constant-index-out-of-range.js fails.

[Yusuke Suzuki]

(In reply to Michael Saboff from comment #7)
> Comment on attachment 378546 [details]
> Patch
> 
> New test stress/string-char-code-at-constant-index-out-of-range.js fails.

Oops, the test is wrong, fixing.

[Yusuke Suzuki]

Created attachment 378565 [details]
Patch

[Michael Saboff]

Comment on attachment 378565 [details]
Patch

r=me

[Yusuke Suzuki]

Committed r249780: <https://trac.webkit.org/changeset/249780>

[Saam Barati]

Comment on attachment 378565 [details]
Patch

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

LGTM too. A couple questions

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2629
> +            if (argumentCountIncludingThis != 2)

Why not “< 2”?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2632
> +            if (m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, Uncountable))

CheckArray is Uncountable?

[Yusuke Suzuki]

Comment on attachment 378565 [details]
Patch

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

Nice

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2629
>> +            if (argumentCountIncludingThis != 2)
> 
> Why not “< 2”?

Doing `< 2` sounds nice. I'll change this for the other DFG nodes (StringCharAt / StringCharCodeAt).

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2632
>> +            if (m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, Uncountable))
> 
> CheckArray is Uncountable?

No, CheckArray emits other thing. Uncountable is checking negative indexes in StringCodePointAt. We should have exit handing for CheckArray here.
And I think StringCharCodeAt and StringCharAt are also missing CheckArray exits. Need to add them.

[Saam Barati]

Comment on attachment 378565 [details]
Patch

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

>>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2629
>>> +            if (argumentCountIncludingThis != 2)
>> 
>> Why not “< 2”?
> 
> Doing `< 2` sounds nice. I'll change this for the other DFG nodes (StringCharAt / StringCharCodeAt).

Yeah I've noticed this pattern in our intrinsic inlining where we check for exact argument count match (or a few potential values), instead of just checking a range. I think checking a range is more complete. (Obviously, it's unlikely these are called with more parameters, but we shouldn't disallow it). When I inline intrinsics, I've been trying to use the "< X" paradigm.

>>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2632
>>> +            if (m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, Uncountable))
>> 
>> CheckArray is Uncountable?
> 
> No, CheckArray emits other thing. Uncountable is checking negative indexes in StringCodePointAt. We should have exit handing for CheckArray here.
> And I think StringCharCodeAt and StringCharAt are also missing CheckArray exits. Need to add them.

👍🏼