Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential new visitor candidate #227

Closed
wants to merge 1 commit into from

Conversation

Schottkyc137
Copy link
Contributor

@Schottkyc137 Schottkyc137 commented Nov 18, 2023

This visitor tries to avoid heap allocation by directly pushing onto the stack and not populating a vec first.

I tried to measure performance but the visitor has a small impact so I'm not sure how reliable this information is. However the number I give here were quite consistent:

  • Without a Visitor: ~460 ms
  • With a Visitor (old implementation): ~525 ms
  • With a Visitor (new implementation): ~490 ms

The change seems to have an effect but it's not really noticeable (only around 30-40 ms). The visitor implementation I used:

struct Test {}

impl Visitor for Test {
    fn visit_ident(&mut self, _node: &Ident, _ctx: &dyn TokenAccess) -> VisitorResult {
        std::hint::black_box(());
        Continue
    }
}

I iterated over each source file in root and ran the visitor on the source file.

This PR is more of an idea concerning #219. That being said, it's fully functional without any API changes (so could theoretically be merged).

@kraigher
Copy link
Member

I would say its not worth the extra complexity for the small gain in performance.

@Schottkyc137
Copy link
Contributor Author

I would say its not worth the extra complexity for the small gain in performance.

What do you mean by extra complexity? The code size is almost the same. This also avoids the heap allocation (at least when iterating over the children)

@kraigher
Copy link
Member

Well complexity is relative. In the simplest code I could imagine for iterating over something we would just use an iterator. But here we need to push to a stack which is an extra argument. Sure it works but it also leaves me thinking if there is not a better way.
I will be away for a few hours now but I will come back and write more about this in the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants