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).
Created attachment 373517 [details] Patch
Created attachment 373518 [details] Patch
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.
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
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.
Created attachment 373524 [details] Patch for landing
Comment on attachment 373524 [details] Patch for landing Clearing flags on attachment: 373524 Committed r247171: <https://trac.webkit.org/changeset/247171>
All reviewed patches have been landed. Closing bug.