Bug 199520 – [WHLSL] Optimize the lexer

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGPU

[Robin Morisset]

Now that we've fixed other bottlenecks in the rest of the compiler, the parser + lexer are once again a major time sink (25% in Myles's measurements).

[Robin Morisset]

Created attachment 373517 [details]
Patch

[Robin Morisset]

Created attachment 373518 [details]
Patch

[EWS Watchlist]

Attachment 373518 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1359:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1567:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1601:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1653:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1708:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1742:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1776:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1809:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 8 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

Comment on attachment 373518 [details]
Patch

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

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLexer.cpp:253
> +        return "LEXING_ERROR";

Why not put a space in it? This is just used for error messages.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLexer.cpp:500
> +    if (isFullyConsumed())

isFullyConsumed calls peek() which I think calls this. Doesn't this have a chance of infinite looping?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLexer.cpp:519
> +//        which means we'll never see a false match. If we see a BMP code unit, we
> +//        really have a BMP code point.

I know you didn't do this, but can we fix the indentation?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLexer.cpp:670
> +    while (auto result = digit(offset))

Cool.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLexer.cpp:-759
> -    if (auto result = string("operator&.", offset))

I don't see where this moved to. How do we lex operator&.foo now?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLexer.h:202
> +        Token ringBuffer[2];

Looks like your patch hasn't been rebased. There's now a m_offsetRingBuffer[2].

Does perf improve if we delete m_offsetRingBuffer[2]? It isn't actually used; it's just a convenience. If perf improves, perhaps we could put it behind a DEBUG preprocessor macro.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:195
> +template <Lexer::Token::Type... types>
> +Optional<Lexer::Token> Parser::tryTypes()

Does this cause code bloat? If so, is the performance gain worth it?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:212
> +template <Lexer::Token::Type... types>
> +auto Parser::consumeTypes() -> Expected<Lexer::Token, Error>

ditto

[Robin Morisset]

Comment on attachment 373518 [details]
Patch

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

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLexer.cpp:500
>> +    if (isFullyConsumed())
> 
> isFullyConsumed calls peek() which I think calls this. Doesn't this have a chance of infinite looping?

Good catch. I will replace isFullyConsumed() here by m_offset == m_stringView.length()

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLexer.cpp:519
>> +//        really have a BMP code point.
> 
> I know you didn't do this, but can we fix the indentation?

Sure.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLexer.cpp:-759
>> -    if (auto result = string("operator&.", offset))
> 
> I don't see where this moved to. How do we lex operator&.foo now?

This is done by 
if (auto result = string("&.", offset))
    return validIdentifier(*result);
on line 772/773

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLexer.h:202
>> +        Token ringBuffer[2];
> 
> Looks like your patch hasn't been rebased. There's now a m_offsetRingBuffer[2].
> 
> Does perf improve if we delete m_offsetRingBuffer[2]? It isn't actually used; it's just a convenience. If perf improves, perhaps we could put it behind a DEBUG preprocessor macro.

This is super confusing: I remember deleting m_offsetRingBuffer in this patch (since all of its content is now directly in the tokens that are part of the normal m_ringBuffer), and I did rebase it just 30mn ago.. and somehow m_offsetRingBuffer is no longer in the diff.
Maybe your patch that added m_offsetRingBuffer got reverted and so the rebase removed that part?

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:195
>> +Optional<Lexer::Token> Parser::tryTypes()
> 
> Does this cause code bloat? If so, is the performance gain worth it?

It probably causes a small amount of code bloat. The code of this function is sufficiently tiny that I don't think it is too bad.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:212
>> +auto Parser::consumeTypes() -> Expected<Lexer::Token, Error>
> 
> ditto

ditto.

[Robin Morisset]

Created attachment 373524 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 373524 [details]
Patch for landing

Clearing flags on attachment: 373524

Committed r247171: <https://trac.webkit.org/changeset/247171>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.