Skip to content

Conversation

@nojaf
Copy link
Contributor

@nojaf nojaf commented Dec 19, 2022

Hi @0101,

In light of #14479, I worked on a little experiment to see what collecting the identifiers from AST would look like.

|             Method |     Mean |     Error |    StdDev |    Gen0 | Allocated |
|------------------- |---------:|----------:|----------:|--------:|----------:|
| FindAllIdentifiers | 3.658 ms | 0.0225 ms | 0.0210 ms | 19.5313 |      6 MB |
Found: 4237 in the lexer
Found: 4243 in AST

On the same machine #14479 gave:

|       Method | IdentCapture |     Mean |    Error |   StdDev | Allocated |
|------------- |------------- |---------:|---------:|---------:|----------:|
| ParseBigFile |        False | 74.48 ms | 1.364 ms | 1.209 ms |  30.14 MB |
| ParseBigFile |         True | 79.18 ms | 1.340 ms | 1.253 ms |  32.75 MB |

so the results are very similar.
The AST traversal code could be improved a bit. I wouldn't claim it is fully tail-recursive 🙈.

Overall, I think this is a bit cleaner in terms of the data the syntax tree carries.

@0101
Copy link
Contributor

0101 commented Dec 19, 2022

Thanks for trying this out @nojaf. It looks fast enough for collecting it, not sure about doing it every time you're searching for something, but could be doable also (most files shouldn't be this big).

But it is a lot of extra code to bring in just for this. That someone would have to deal with every time something changes in the AST (probably you 😄).

I'd probably be more in favor of the flag to enable the collection as it is.

@nojaf
Copy link
Contributor Author

nojaf commented Dec 20, 2022

not sure about doing it every time you're searching for something

That could be cached.

But it is a lot of extra code to bring in just for this. That someone would have to deal with every time something changes in the AST

There is no denying that. Although the amount of code is more of an infrastructure problem.
Imagine SyntaxVisitorBase having a richer API, we only need to implement a callback or two and can capture all the idents that way.

I'd probably be more in favour of the flag to enable the collection as it is.

I can live with that.

@nojaf nojaf closed this Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants