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

Slightly speed up the all filter operator #7702

Merged
merged 1 commit into from Sep 24, 2023

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Aug 29, 2023

The all filter operator has shortcuts to optimise common patterns like [all[shadows+tiddlers]] or [all[tiddlers]]. In those cases, the filter operator function returns early and never uses the result linked list that was created, so it's immediately garbage-collected. This PR attempts to speed up the result of calling all in those common cases by only creating that linked list if it's actually going to be used, avoiding that garbage collection in the common cases that are optimised to return early.

The `all` filter operator has shortcuts to optimise common patterns like
`[all[shadows+tiddlers]]` or `[all[tiddlers]]`. In those cases, the
filter operator function returns early and never uses the `result`
linked list that was created, so it's immediately garbage-collected.
Let's delay creating it until we know it's actually going to be used.
@vercel
Copy link

vercel bot commented Aug 29, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
tiddlywiki5 ✅ Ready (Inspect) Visit Preview Aug 29, 2023 4:17pm

@rmunn
Copy link
Contributor Author

rmunn commented Aug 29, 2023

All unit tests still pass with this PR. I have not yet figured out how to measure the performance benefits you get from this optimisation. I expect it to be relatively small, as creating a $tw.utils.LinkedList object allocates about three objects (the linked list and its next and prev properties, which are LLMap objects) with about 5-6 allocated properties total. That's not a huge amount of memory to churn: a couple dozen bytes at most. However, the [all[shadows+tiddlers]] filter is used all over the place in TiddlyWiki's internals, and every one of those filters is currently allocating a LinkedList and then immediately throwing it away without using it. So those dozens of bytes, and the work the Javascript garbage collector has to do to keep track of them, will eventually add up to some measurable amount of time, I just don't know how large a wiki will need to be before it's noticeable.

@rmunn
Copy link
Contributor Author

rmunn commented Aug 29, 2023

@flibbles - You originally created #5362 in order to optimise the performance of the all operator, and this PR is simply building on your work a little. In that PR, you wrote "There's more room for improvement by cutting down all that garbage collection ...", and I think this is a small step in that direction. Since you already have a test wiki set up for performance measuring (or at least, you did at the time you created #5362), would you be interested in using it to measure the speedup obtained by this small change?

@flibbles
Copy link
Contributor

I do not have a test wiki set up. Your change seems pretty simple. It either helps or it does nothing. It won't hurt. I'm not sure it needs a run of performance testing.

@Jermolene
Copy link
Owner

Thanks for your patience @rmunn

@Jermolene Jermolene merged commit 780e5d3 into Jermolene:master Sep 24, 2023
4 checks passed
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.

None yet

3 participants