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

Improve the performance of Index.add. #208

Closed
wants to merge 1 commit into from
Closed

Conversation

d0ugal
Copy link
Contributor

@d0ugal d0ugal commented Mar 4, 2016

The loops in Index.add can easily be called hundreds of thousands of times for
large documents. Larger documents can perform particularly badly1 under
Chrome. Removing the usage of reduce and filter in favour of inline native for
loops can lead to a 20-30% performance increase in Chrome. Firefox also sees a
similar but much smaller impovement (~5%).

The loops in Index.add can easily be called hundreds of thousands of times for
large documents. Larger documents can perform particularly badly[1] under
Chrome. Removing the usage of reduce and filter in favour of inline native for
loops can lead to a 20-30% performance increase in Chrome. Firefox also sees a
similar but much smaller impovement (~5%).

[1]: mkdocs/mkdocs#859 (comment)
@d0ugal
Copy link
Contributor Author

d0ugal commented Mar 7, 2016

Would it be useful for me to create a jsperf test for this (as #163 did) or are the included benchmarks good enough?

The new benchmark I added isn't ideal since it causes GitHub to hide the diff. I'd be happy to do this any other way if you have any suggestions. Perhaps the test data needs to be moved to a JSON file.

@olivernn
Copy link
Owner

olivernn commented Mar 8, 2016

Thanks for taking a look at this, from the tests I did there was a significant performance improvement, which is always great!

I think that the new benchmark is fine for now, having the fixture in a separate file might make browsing the tests etc easier but its no show stopper.

I've included your change in the latest version, let me know if you have any issues, and thanks again for your help, much appreciated!

@olivernn olivernn closed this Mar 8, 2016
@d0ugal
Copy link
Contributor Author

d0ugal commented Mar 9, 2016

Thanks very much!

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