Bug 221593 – [JSC] Make JSON.parse faster by using table for fast string parsing

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

[Yusuke Suzuki]

[JSC] Make JSON.parse faster by using table for fast string parsing

[Yusuke Suzuki]

Created attachment 419674 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 419674 [details]
Patch

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

> Source/JavaScriptCore/runtime/LiteralParser.cpp:437
> +enum StringCharacter : uint8_t { Slow, Fast, };
> +static constexpr const StringCharacter safeStringLatin1CharactersInStrictJSON[256] = {

What does slow & fast mean? StringCharacter is also a very generic term to use here.
Maybe renaming this to safeLatin1CharacterInStrictJSON and using boolean would be better.
If we wanted to use enum class maybe something like this will be more appropriate:
enum class JSONLiteralCharacterType : uint8_t { Safe, NotSafeInStrictMode };

> Source/JavaScriptCore/runtime/LiteralParser.cpp:839
> +        return (c >= ' ' && c != '\\' && c != terminator) || (c == '\t');

Isn't this really just safeStringLatin1CharactersInStrictJSON[c] == StringCharacter::Fast || c == '\t'?

> Source/JavaScriptCore/runtime/LiteralParser.cpp:850
> +        return (c >= ' ' && isLatin1(c) && c != '\\' && c != terminator) || (c == '\t');

Ditto.

[Geoffrey Garen]

r=me

> Source/JavaScriptCore/runtime/LiteralParser.cpp:438
> +// 256 Latin-1 codes
> +enum StringCharacter : uint8_t { Slow, Fast, };
> +static constexpr const StringCharacter safeStringLatin1CharactersInStrictJSON[256] = {
> +/*   0 - Null               */ StringCharacter::Slow,

I'm curious: Why do we call these 'Fast' and 'Slow' instead of 'Safe' and 'Unsafe'? I'm not familiar with this distinction -- I'm just observing that the table name is "safe..." and not "fast....".

> Source/JavaScriptCore/runtime/LiteralParser.cpp:837
> +    if constexpr (set == SafeStringCharacterSet::Strict)
> +        return safeStringLatin1CharactersInStrictJSON[c] == StringCharacter::Fast;

Can we make the table store booleans, so we can just return the boolean instead of doing an additional comparison against StringCharacter::Fast?

(Alternatively, I suppose we could change the return value from bool to StringCharacter.)

[Yusuke Suzuki]

Comment on attachment 419674 [details]
Patch

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

>> Source/JavaScriptCore/runtime/LiteralParser.cpp:437
>> +static constexpr const StringCharacter safeStringLatin1CharactersInStrictJSON[256] = {
> 
> What does slow & fast mean? StringCharacter is also a very generic term to use here.
> Maybe renaming this to safeLatin1CharacterInStrictJSON and using boolean would be better.
> If we wanted to use enum class maybe something like this will be more appropriate:
> enum class JSONLiteralCharacterType : uint8_t { Safe, NotSafeInStrictMode };

OK, I'll just change this to boolean.

>> Source/JavaScriptCore/runtime/LiteralParser.cpp:839
>> +        return (c >= ' ' && c != '\\' && c != terminator) || (c == '\t');
> 
> Isn't this really just safeStringLatin1CharactersInStrictJSON[c] == StringCharacter::Fast || c == '\t'?

terminator can be different in non-Strict-JSON (JSONP etc., not an actual JSON case). So still this is a bit different.

>> Source/JavaScriptCore/runtime/LiteralParser.cpp:850
>> +        return (c >= ' ' && isLatin1(c) && c != '\\' && c != terminator) || (c == '\t');
> 
> Ditto.

Changed.

[Yusuke Suzuki]

Created attachment 419676 [details]
Patch

[Yusuke Suzuki]

animations/keyframe-pseudo-shadow.html is unrelated.

[Yusuke Suzuki]

Committed r272570: <https://trac.webkit.org/changeset/272570>

[Radar WebKit Bug Importer]

<rdar://problem/74134694>

[Yusuke Suzuki]

8% improvement in JetStream2/json-parse-inspector

[Ryosuke Niwa]

(In reply to Yusuke Suzuki from comment #9)
> 8% improvement in JetStream2/json-parse-inspector

Nice!