Use generator instead of list expand or add method #1225
Conversation
Generator is memory efficient approach.
|
I like this change! Review: You can turn a few more of the "yield"s to "yield from"s. |
Also improve performance of "iter_subtrees_topdown" Performance of "iter_subtrees_topdown" method reduce as size of tree increases. Using instance of list method improve the performance.
|
@erezsh Done! |
|
What I meant is that you don't need any for loop.. yield from (i for i in ('\t', '%s' % (self.children[0],), '\n'))can be yield from ('\t', '%s' % (self.children[0],), '\n')etc. Although this particular expression can be rewritten as a single string format. Give it another try if you like. Otherwise no problem, I'll do the finishing touches myself. |
|
Thank you for review, I change few lines, use yield with f-string method. |
| @@ -149,13 +148,15 @@ def iter_subtrees_topdown(self): | |||
| Iterates over all the subtrees, return nodes in order like pretty() does. | |||
| """ | |||
| stack = [self] | |||
| stack_append = stack.append | |||
There was a problem hiding this comment.
I assume you added this for performance. Did you benchmark it on a realistic use-case?
There was a problem hiding this comment.
Yes, I'm working on one language module and use lark to create grammar.
After changing append and pop method calling approach, performance improve.
There was a problem hiding this comment.
Did you try also changing Tree to a local variable (_Tree or such)?
There was a problem hiding this comment.
Not such type of thing, but I create child class of it, and enhance some part for better performance.
Also working on multi-threading, with cythonise some part of code and logic.
There was a problem hiding this comment.
I meant specifically inside this function, to skip the global lookup.
There was a problem hiding this comment.
Also, you're aware of https://github.com/lark-parser/lark_cython?
Yes.
I recommend, we should start implement lark in C++ or Rust, so with WASM and Cython (or Pybind11), we can use optimise version of it in Python and JS world.
There was a problem hiding this comment.
Having Lark implemented in Rust would be awesome. But I don't have the time to do it myself.
Anyway, there's still a lot of speed to be gained with lark-cython, it's still using a lot of Python objects when it doesn't have to.
There was a problem hiding this comment.
I'm interested to take both the tasks, if you allow.
There was a problem hiding this comment.
You're very welcome to do so!
Just keep in mind I will only merge code of high quality, i.e. readable, tested, etc.
There was a problem hiding this comment.
Sure, I will take care of it.
|
Thanks! |
We can use generator to create pretty string of Tree in place of list. This PR reduce memory usage.