[webkit-reviews] review granted: [Bug 192890] [WHLSL] Parsing and lexing the standard library is slow : [Attachment 370805] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Robin Morisset
<rmorisset at apple.com>'s request for review:
Bug 192890: [WHLSL] Parsing and lexing the standard library is slow
https://bugs.webkit.org/show_bug.cgi?id=192890

Attachment 370805: Patch

https://bugs.webkit.org/attachment.cgi?id=370805&action=review




--- Comment #7 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 370805
  --> https://bugs.webkit.org/attachment.cgi?id=370805
Patch

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

> Source/WebCore/ChangeLog:1767
> +2019-05-28  Robin Morisset  <rmorisset at apple.com>

Changelogs should go at the top of the file.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLexer.h:184
> +	   m_ringBufferIndex = !m_ringBufferIndex;

Nit: this could be made more clear by saying (m_ringBufferIndex + 1) % 2. Maybe
one day we'll want to have 3 tokens of lookahead.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLexer.h:195
> +	   return m_ringBuffer[!m_ringBufferIndex];

Ditto

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLexer.h:205
>      State state() const

We should add a FIXME with a link to a bugzilla bug about removing the last of
the backtracking, at which point we can delete these functions. The FIXME could
go here or it could go where the backtracking occurs (or both).

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:58
> +	   auto token = m_lexer.peek();

Can't you just call peek( ) instead of going through the parser? Having fewer
funnel points is better.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:87
> +	       ASSERT(m_mode == Mode::StandardLibrary);

Shouldn't this be a parse error, rather than an ASSERT()? We can't stop authors
from putting the token "native" in their source.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:365
> +    if (m_lexer.peek()->type != Lexer::Token::Type::Identifier ||
m_lexer.peekFurther()->type == Lexer::Token::Type::FullStop) {

Did you mechanically generate these tokens using a tool? Or did you figure it
out by looking through the grammar yourself?

If we add a new type of token, are we going to have to modify lots of places
throughout the parser?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:382
> +    auto greaterThanSign = tryType(Lexer::Token::Type::GreaterThanSign);
> +    if (greaterThanSign)
> +	   return typeArguments;

Should there be a macro for this pattern too? It's used in quite a few places.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:703
> +    while (auto next = tryType(Lexer::Token::Type::Qualifier)) {

The rare "variable declaration in a while loop condition"! I like it!

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:744
>	   auto structureElement =
backtrackingScope<Expected<AST::StructureElement, Error>>([&]() {

😔

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:986
> +	   return parseVertexFragmentFunctionDeclaration();

Nit: VertexOrFragment

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1091
>      auto parseRemainder = [&](Variant<AST::VariableDeclarationsStatement,
UniqueRef<AST::Expression>>&& initialization) -> Expected<AST::ForLoop, Error>
{

With the new design, is it possible to get rid of this lambda and put its
contents directly into parseForLoop()?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1335
> +    auto origin = peek();
> +    if (!origin)
> +	   return Unexpected<Error>(origin.error());

Can be macro-ized?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1667
> +	   Vector<UniqueRef<AST::Expression>> callArguments;
> +	   callArguments.append(WTFMove(previous));
> +	   callArguments.append(WTFMove(*next));

Can we say "callArguments { WTFMove(previous), WTFMove(*next) }"?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1718
> +	   Vector<UniqueRef<AST::Expression>> callArguments;
> +	   callArguments.append(WTFMove(previous));
> +	   callArguments.append(WTFMove(*next));

Ditto

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1752
> +	   Vector<UniqueRef<AST::Expression>> callArguments;
> +	   callArguments.append(WTFMove(previous));
> +	   callArguments.append(WTFMove(*next));

Ditto

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1787
> +	   Vector<UniqueRef<AST::Expression>> callArguments;
> +	   callArguments.append(WTFMove(previous));
> +	   callArguments.append(WTFMove(*next));

Ditto

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1827
> +	       if (isEffectful)
> +		   *isEffectful = true;

If possible, output values should be part of the return type, not out-params.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1913
> +	       return fail("Could not read next token in the callExpression
case of parsePossibleSuffix()"_str, TryToPeek::No);

Isn't a better error message "the term isn't effectful" or "need a semicolon"
or something like that?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.h:123
> +	   void dump(PrintStream& out) const
> +	   {
> +	       out.print(error);
> +	   }

Is this ever called? Maybe it should be behind #ifndef NDEBUG

Also, possibly rename to dumpError()