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

perf: use swiss map for the object cache #677

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Conversation

peter7891
Copy link
Contributor

@peter7891 peter7891 commented Mar 30, 2023

This PR uses swiss map for the object cache.
Detailed benchmarks in this blog https://www.dolthub.com/blog/2023-03-28-swiss-map/

I've verified these are correct.
We should consider using this map in other places as well.
For example, in the implementation of the builtin map in Gno.

@peter7891 peter7891 requested a review from a team as a code owner March 30, 2023 13:14
@peter7891 peter7891 requested review from jaekwon and moul March 31, 2023 10:29
@moul
Copy link
Member

moul commented Apr 1, 2023

Hey @peter7891, could you provide more detailed benchmarks for our code, including measuring the execution time of a Gno contract? Also, do you have any suggestions for a benchmarking framework that we can use to centralize our performance history and optimization documentation?

Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends on #689

@peter7891
Copy link
Contributor Author

Let's wait until we have the tool, to merge this.
In the mean time, on a heap with a thousand objects.

Fetching objects on master

BenchmarkWriteReadLargeHeap-8                 70          17094023 ns/op

Fetching objects on this branch

BenchmarkWriteReadLargeHeap-8                 62          14150240 ns/op

This is a large improvement and it gets better tha larger the heap is.

@moul
Copy link
Member

moul commented May 6, 2023

Following a recent discussion, we have reconsidered our position, let's merge PRs freely for now, but keep the perf dashboard (#689) as a future goal.

Please provide benchmarks with larger heap size to help achieve this. Thanks.

@moul moul marked this pull request as draft May 6, 2023 16:56
@peter7891
Copy link
Contributor Author

Following a recent discussion, we have reconsidered our position, let's merge PRs freely for now, but keep the perf dashboard (#689) as a future goal.

Please provide benchmarks with larger heap size to help achieve this. Thanks.

I added benchmarks for the maps themselves.

BenchmarkSwissMap
BenchmarkSwissMap-10    	15304465	        76.86 ns/op
BenchmarkMap
BenchmarkMap-10    	13462977	        90.45 ns/op

In the larger context of the heap, it makes less of a different because the other operations around it add a lot more runtime costs. So i am not sure if making this change is worth making now.
Further changes will be made when the GC/runtime PR gets merged.
I think, it might be better to wait until then and see if this is even necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Status: 🌟 Wanted for Launch
Development

Successfully merging this pull request may close these issues.

3 participants