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

Rewrite Descendants iteratively #288

Merged
merged 7 commits into from
Jan 19, 2019

Conversation

MihaZupan
Copy link
Collaborator

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:

Method Mean Gen 0/1k Op Gen 1/1k Op Gen 2/1k Op Allocated Memory/Op
Markdig 212.9 ms 99333.3333 - - 74.89 MB
Markdig_Descendants 228.8 ms 98000.0000 2000.0000 - 78.22 MB

Recursive:

Method Mean Gen 0/1k Op Gen 1/1k Op Gen 2/1k Op Allocated Memory/Op
Markdig 215.8 ms 99333.3333 - - 74.89 MB
Markdig_Descendants 242.9 ms 98000.0000 119000.0000 6000.0000 104.3 MB

Where Markdig is only parsing to the AST while Markdig_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> of ContainerInline and ContainerBlock 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.

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>();
Copy link
Owner

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

Copy link
Collaborator Author

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?

Copy link
Owner

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 👍

@xoofx
Copy link
Owner

xoofx commented Jan 19, 2019

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 👍

@MihaZupan
Copy link
Collaborator Author

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

@xoofx xoofx merged commit e643203 into xoofx:master Jan 19, 2019
@xoofx
Copy link
Owner

xoofx commented Jan 19, 2019

Thanks for this!

@MihaZupan MihaZupan deleted the rewrite-descendants-search branch March 11, 2019 17:33
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