Skip to content

Conversation

segevfiner
Copy link
Contributor

@segevfiner segevfiner commented Jul 9, 2024

Add iterator methods to Query

Still need to do capturesIter and CapturesIterator, typings, etc.

I'm not that happy about the code duplication though... We could probably implement matches and captures in terms of matchesIter and capturesIter to reduce it. But that might be slower due to all the additional calls that would entail between C++ and JS, and transferring the nodes one by one.

P.S. LookaheadIterator[@@iterator] can probably be implemented in C++, and ts_query_cursor can probably be allocated on Query instead of in AddonData if we want too.

Fixes #178

@segevfiner segevfiner changed the title WIP: feat: Add iterator methods to Query feat: Add iterator methods to Query Jul 10, 2024
@segevfiner segevfiner marked this pull request as ready for review July 10, 2024 18:52
@amaanq
Copy link
Member

amaanq commented Aug 17, 2024

I think it'd make more sense to just implement matches and captures with IterableIterator by default, what do you think?

@segevfiner
Copy link
Contributor Author

I'm worried about backwards compat abd performance. Which is why I kept the existing ones for now.

@amaanq
Copy link
Member

amaanq commented Aug 17, 2024

If performance is a concern, why even add this then? What's the benefit/purpose exactly? (really curious because I'm all for iterators)

@segevfiner
Copy link
Contributor Author

segevfiner commented Aug 17, 2024

Memory consumption. As using iterators means we don't need to hold all results in memory at once. We still need to measure if it's really significantly slower or not. I'm not sure if it's really slower. But if it is, it is always possible to chunk the iteration internally so we don't transfer one by one.

@amaanq amaanq force-pushed the master branch 3 times, most recently from 4e43908 to ff76451 Compare November 10, 2024 22:38
@tree-sitter tree-sitter deleted a comment Jan 30, 2025
@maxbrunsfeld
Copy link
Contributor

From my experience, using iterators is going to be much less efficient, because there are many JS -> C++ calls, not just one. I don't think we should make this change at all, unless there is some very specific benchmarks demonstrating that it solves a performance problem.

@segevfiner
Copy link
Contributor Author

Very large result sets are one reason. But we may need batching of results for this to be efficient. And benchmarks obviously.

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.

Consider making Query matches/captures iterators
3 participants