Skip to content
This repository was archived by the owner on Feb 6, 2025. It is now read-only.

Try to optimize the expression parser #54

Open
hadronized opened this issue Nov 20, 2018 · 2 comments
Open

Try to optimize the expression parser #54

hadronized opened this issue Nov 20, 2018 · 2 comments
Labels
enhancement question & discussion A question or a discussion about a topic

Comments

@hadronized
Copy link
Owner

Some benchmarks are needed to ensure what the problem is, but I’m pretty sure (given the current nom-3 implementation) that we have a lot of failures and retries.

@hadronized hadronized added enhancement question & discussion A question or a discussion about a topic labels Nov 20, 2018
@topecongiro
Copy link
Contributor

I have observed that nested parens will significantly slow down the parser.

void main ()
{
  (((((((1.0)))))));
}

Parsing the above file with the release build takes about 10 seconds on iMac.

Interestingly, there seems to be no problem when parsing nested function calls:

void main ()
{
  foo(foo(foo(foo(foo(foo(foo(1.0)))))));
}

Parsing the above snippet takes less than 10 ms on the same machine.

@alixinne
Copy link
Contributor

alixinne commented Feb 1, 2021

I've been exploring parsing GLSL expressions with the LALRPOP parser generator, combined with Logos as the lexer generator. Building the same AST is around 1300x (not a typo) faster with a lalrpop/logos than the current nom combinators:

    Finished bench [optimized] target(s) in 0.07s
Parse expression/lalrpop                                                                             
                        time:   [3.9253 us 3.9293 us 3.9337 us]
Parse expression/glsl   time:   [5.1026 ms 5.1166 ms 5.1302 ms]      

This should be taken with a grain of salt, since this is a very simple test (the nested_parentheses one), and running on my branch for #129. I'll see if I can validate those improvements over some other examples and against upstream, but it might be interesting to consider rewriting the parser.

Considering the other issues I've been working on, especially #129, adding location information to the AST would require changing a lot in the parser definition, so we might as well rewrite the parser with lalrpop and support span information at the same time.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement question & discussion A question or a discussion about a topic
Projects
None yet
Development

No branches or pull requests

3 participants