Bug 201673

Summary:

[JSC] Add StringCodePointAt intrinsic

Product:

WebKit

Reporter:

Yusuke Suzuki <ysuzuki>

Component:

JavaScriptCore

Assignee:

Yusuke Suzuki <ysuzuki>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	msaboff: review+

Yusuke Suzuki

Reported 2019-09-10 19:17:45 PDT

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'

Attachments
Patch (28.75 KB, patch) 2019-09-10 22:07 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (28.79 KB, patch) 2019-09-10 22:19 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (41.10 KB, patch) 2019-09-11 01:42 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (41.11 KB, patch) 2019-09-11 09:27 PDT, Yusuke Suzuki	msaboff: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2019-09-10 19:18:07 PDT

<rdar://problem/55246768>

Yusuke Suzuki

Comment 2 2019-09-10 21:57:49 PDT

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

Yusuke Suzuki

Comment 3 2019-09-10 22:07:32 PDT

Created attachment 378538 [details] Patch

Yusuke Suzuki

Comment 4 2019-09-10 22:19:09 PDT

Created attachment 378539 [details] Patch

Yusuke Suzuki

Comment 5 2019-09-11 01:42:19 PDT

Created attachment 378546 [details] Patch

EWS Watchlist

Comment 6 2019-09-11 03:57:21 PDT

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 7 2019-09-11 08:02:28 PDT

Comment on attachment 378546 [details] Patch New test stress/string-char-code-at-constant-index-out-of-range.js fails.

Yusuke Suzuki

Comment 8 2019-09-11 09:26:28 PDT

(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

Comment 9 2019-09-11 09:27:00 PDT

Created attachment 378565 [details] Patch

Michael Saboff

Comment 10 2019-09-11 15:18:36 PDT

Comment on attachment 378565 [details] Patch r=me

Yusuke Suzuki

Comment 11 2019-09-11 15:37:00 PDT

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

Saam Barati

Comment 12 2019-09-12 11:05:14 PDT

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 13 2019-09-12 12:10:22 PDT

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 14 2019-09-12 14:26:27 PDT

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. 👍🏼

Note You need to log in before you can comment on or make changes to this bug.