-
-
Notifications
You must be signed in to change notification settings - Fork 455
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
Rewrite Descendants iteratively #288
Conversation
if (!(markdownObject is ContainerBlock) && !(markdownObject is ContainerInline)) yield break; | ||
|
||
// TODO: A single Stack<(MarkdownObject block, bool push)> when ValueTuples are available | ||
Stack<MarkdownObject> stack = new Stack<MarkdownObject>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use instead a small internal struct for that, that's fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing with a struct in a single stack I go from a 3.3 to a 4.5 MB increase in allocations. Want me to commit it anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Han, yes, because of the type alignment, the byte will take actually an IntPtr... ok, We can keep two separate stacks then 👍
Yeah, I remember that I was very lazy for this one, so I'm glad that you are optimizing this. Indeed, that's much better 👍 |
I've tried to rewrite it before for fun, but failed at keeping the order (didn't think to keep the extra bool). I was just surprised to see this method show up when doing some profiling yesterday |
Thanks for this! |
I rewrote the
Descendants
extension methods to eliminate recursion. The order of returned elements is kept the same.This comes with a significant memory allocation reduction when using
Descendants
over the entire AST like I do in my project.Benchmarking between the recursive and iterative approaches I get this:
Iterative:
Recursive:
Where
Markdig
is only parsing to the AST whileMarkdig_Descendants
parses to AST and loops over descendants once.I have tested that the order is not modified when calling descending anywhere in the test suite.
I don't have tests for
Descendants<T>
ofContainerInline
andContainerBlock
as they appear to not be used anywhere in Markdig itself, but I have included iterative implementations for those as well.In the future, a few bytes could be shaved off by using ValueTuple instead of two stacks.