Bug 194466 - [JSC] String.fromCharCode's slow path always generates 16bit string
Summary: [JSC] String.fromCharCode's slow path always generates 16bit string
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-08 18:24 PST by Yusuke Suzuki
Modified: 2019-02-09 15:16 PST (History)
7 users (show)

See Also:


Attachments
Patch (4.92 KB, patch)
2019-02-08 18:33 PST, Yusuke Suzuki
keith_miller: review+
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (9.62 MB, application/zip)
2019-02-08 20:31 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-02-08 18:24:11 PST
[JSC] String.fromCharCode's slow path always generates 16bit string
Comment 1 Yusuke Suzuki 2019-02-08 18:33:06 PST
Created attachment 361577 [details]
Patch
Comment 2 Keith Miller 2019-02-08 18:58:32 PST
Comment on attachment 361577 [details]
Patch

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

r=me with small fix.

> Source/JavaScriptCore/ChangeLog:10
> +        and even worse, taints ropes 16bit if 16bit string is included in the given rope. We find that acorn-wtb

Typo: We find => We found. Or if you want to take more credit: I found.

> Source/JavaScriptCore/ChangeLog:12
> +        16bit string. However, only few strings are actually 16bit strings. This patch attempts to make 8bit string

Typo: only few => only a few

> Source/JavaScriptCore/runtime/StringConstructor.cpp:91
> +        if (UNLIKELY(character & 0xFF00)) {

Can we use or create a named WTF function for this?
Comment 3 Yusuke Suzuki 2019-02-08 19:26:49 PST
Comment on attachment 361577 [details]
Patch

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

>> Source/JavaScriptCore/runtime/StringConstructor.cpp:91
>> +        if (UNLIKELY(character & 0xFF00)) {
> 
> Can we use or create a named WTF function for this?

Yeah, I added `is8BitCharacter(ch)`, since ASCIICType::isASCII(ch) is different from this definition. (It does not allow the upper most bit is set, since it is precisely out of ASCII range).
Comment 4 Yusuke Suzuki 2019-02-08 20:13:21 PST
Comment on attachment 361577 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:10
>> +        and even worse, taints ropes 16bit if 16bit string is included in the given rope. We find that acorn-wtb
> 
> Typo: We find => We found. Or if you want to take more credit: I found.

Fixed.

>> Source/JavaScriptCore/ChangeLog:12
>> +        16bit string. However, only few strings are actually 16bit strings. This patch attempts to make 8bit string
> 
> Typo: only few => only a few

Fixed.
Comment 5 Keith Miller 2019-02-08 20:15:05 PST
(In reply to Yusuke Suzuki from comment #3)
> Comment on attachment 361577 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361577&action=review
> 
> >> Source/JavaScriptCore/runtime/StringConstructor.cpp:91
> >> +        if (UNLIKELY(character & 0xFF00)) {
> > 
> > Can we use or create a named WTF function for this?
> 
> Yeah, I added `is8BitCharacter(ch)`, since ASCIICType::isASCII(ch) is
> different from this definition. (It does not allow the upper most bit is
> set, since it is precisely out of ASCII range).

I think we should actually call it isLatin1. IIUC, that's the most precise term.
Comment 6 Build Bot 2019-02-08 20:31:58 PST
Comment on attachment 361577 [details]
Patch

Attachment 361577 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11087027

New failing tests:
imported/w3c/web-platform-tests/webrtc/simplecall.https.html
Comment 7 Build Bot 2019-02-08 20:31:59 PST
Created attachment 361591 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 8 Yusuke Suzuki 2019-02-08 20:40:28 PST
Committed r241233: <https://trac.webkit.org/changeset/241233>
Comment 9 Radar WebKit Bug Importer 2019-02-08 20:41:30 PST
<rdar://problem/47938406>
Comment 10 Darin Adler 2019-02-09 11:37:50 PST
(In reply to Keith Miller from comment #5)
> I think we should actually call it isLatin1. IIUC, that's the most precise
> term.

There are isLatin1 functions in Lexer.cpp already. This patch adds a new one. Could we consolidate?
Comment 11 Yusuke Suzuki 2019-02-09 14:42:39 PST
(In reply to Darin Adler from comment #10)
> (In reply to Keith Miller from comment #5)
> > I think we should actually call it isLatin1. IIUC, that's the most precise
> > term.
> 
> There are isLatin1 functions in Lexer.cpp already. This patch adds a new
> one. Could we consolidate?

Oh, nice! I'll land it in a follow-up patch.
Comment 12 Yusuke Suzuki 2019-02-09 15:16:08 PST
Committed r241246: <https://trac.webkit.org/changeset/241246>