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

Cache for Definitions #507

Merged
merged 4 commits into from
Sep 6, 2016
Merged

Cache for Definitions #507

merged 4 commits into from
Sep 6, 2016

Conversation

poke1024
Copy link
Contributor

This continues #501 with a more general approach.

Depending on the situation, the performance gain with this seems to be between 5% and 15%:

before.txt
after.txt

@sn6uv
Copy link
Member

sn6uv commented Aug 29, 2016

rather than throwing out the whole cache each time, could we pass a name argument to clear_cache and only clear the conflicting items?

@poke1024
Copy link
Contributor Author

So I tried to limit the cache clears. It's a bit tricky since clearing A also needs to clear XAand clearingXA also needs to clear A, and so on.

The new approach is still very on the conservative side and is clearing more than needed (namely a stripped symbol's name across all contexts) and it doesn't try to identify a minimal set. We could try to be still smarter about it, but it's tricky, and since the number of gets should be drastically higher than the number of sets, the occasional wrong clears are, I believe, rare, and I'm a bit too lazy to think through all the border cases right now.

This new implementation is never slower than the previous cache implementation, and it's another 10% faster in some cases:

caching-new.txt

@sn6uv
Copy link
Member

sn6uv commented Sep 6, 2016

Did some testing and got quite a significant speedup: 70s -> 47s. I'm not completely happy with how complicated the Definitions object is becoming but in this case I think the trade-off is worthwhile.

@sn6uv sn6uv merged commit cc5ef4f into mathics:master Sep 6, 2016
@sn6uv sn6uv mentioned this pull request Sep 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants