8000 Multi Get and Multi Set by Winnetoe24 · Pull Request #4 · arp242/zcache · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Multi Get and Multi Set #4

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: v2
Choose a base branch
from
Open

Multi Get and Multi Set #4

wants to merge 1 commit into from

Conversation

Winnetoe24
Copy link

No description provided.

@arp242
Copy link
Owner
arp242 commented Dec 13, 2023

Is there any reason in particular you used two slices in MultiGet() rather than a slice with a struct? It seems to me that's quite a bit more convenient, and this can also work for setting multiple keys:

func (c *cache[K, V]) MultiSet(vals ...struct {
	k K
	v V
}) {
	c.mu.Lock()
	for _, v := range vals {
		c.set(v.k, v.v, c.defaultExpiration)
	}
	c.mu.Unlock()
}

@Winnetoe24
Copy link
Author

i personaly found the resuling code, that uses the function more readable. But this is personal preference. I can rework this with structs if you like

@arp242
Copy link
Owner
arp242 commented Dec 14, 2023

It seems to me that juggling around two variables for one thing is rather awkward. Then again I also don't think my version is super-great either (it was just the quickest to write – I mostly wanted to compare performance to see if it's worth it after our Twitter chat).

Since I really want to avoid introducing incompatible changes in the future I'd like to get this right – I need to play around and think a bit to see what works, so let me get back to you.

@arp242
Copy link
Owner
arp242 commented Dec 14, 2023

Also: there's quite a few other operations where it makes sense to operate on more than one key. Why do Get() and Set() have "Multi versions" but not Add() or Delete() or Touch() or GetWithExpire() or any of the others?

Adding Multi-variants for each one would be a bit much, but I'm wonder if there's nice way to make a more generic API that extends to more operations – for example maybe something like (and just thinking out loud here):

// Returns []struct{..} or two slices.
vals := mycache.Multi(key1, key2).Get()

mycache.Multi(key1, key2).Delete()

mycache.Multi(key1, key2).Set("val1", "val2")

mycache.Multi(key1, key2).SetWithExpire(expire, "val1", "val2")

mycache.Multi(key1, key2).Rename("newname1", "newname2")

Implementation is more work, but I don't mind doing the work on that. Getting the API right is what I'm most concerned with.

@Winnetoe24
Copy link
Author
Winnetoe24 commented Dec 14, 2023 •
edited by arp242
Loading

That might be a good idea. I tested 2 return types: ([]V, []bool), []struct{v V, exists bool} in benchmarks. I modified your benchmark. The return the get-method is iterated over and if a value doesn't exists time.Sleep(time.Nanosecond) is used to simulate loading the value and the value is set to "".

goos: windows
goarch: amd64
pkg: zgo.at/zcache/v2
cpu: AMD Ryzen 7 5700U with Radeon Graphics
BenchmarkGetMulti/get
BenchmarkGetMulti/get-16                       2        6196558400 ns/op
BenchmarkGetMulti/mget-16                      2        6155529550 ns/op
BenchmarkGetMulti/mgetStruct-16                4        2641001275 ns/op
PASS

As you can see, my idea with 2 slices completely ruins the performance, therefore the struct version should be the right return type.

@arp242
Copy link
Owner
arp242 commented Dec 14, 2023 •
edited
Loading

Hm, I get quite different benchmark results?

BenchmarkGetMulti/get-8                    62580             17229 ns/op               0 B/op          0 allocs/op
BenchmarkGetMulti/mget-8                  112527             12081 ns/op           16384 B/op          1 allocs/op
BenchmarkGetMulti/mget2-8                 114786             10299 ns/op           10368 B/op          2 allocs/op

I put the benchmark in https://github.com/arp242/zcache/tree/mget

My benchmark is a lot simpler though.

@Winnetoe24
Copy link
Author

Your Benchmark only tests the method and misses the access of the method retrieved.
I find this important since you normally iterate through the information and load information the cache missed. My Tests simulates the load through time.sleep.
Depending on the result time, the access of the data differs.
My results might also be influenced by my cpu since it's a 15w Laptop CPU, which ran on battery.

@Winnetoe24
Copy link
Author

I will further test tomorrow

@arp242
Copy link
Owner
arp242 commented Dec 17, 2023

I will further test tomorrow

It's fine, don't bother – it's not hugely important.

arp242 added a commit that referenced this pull request Jan 3, 2024
@arp242
Add Keyset
4d19193 
A "keyset" is a set of keys for a cache, similar to how a slice is a
"view" on an array. This allows reasonably convenient and efficient
operations on multiple keys.

Doing it with a different type like this gives us the advantage that we
can reasonably easily add a "multi" version for all methods.

Fixes #4
@arp242 arp242 mentioned this pull request Jan 3, 2024
Open
@arp242
Copy link
Owner
arp242 commented Jan 3, 2024

This seems to work reasonably well: #5 – what do you think?

Keyset.Set() is up to ~60% faster, and Keyset.Get() up to ~40%.

Need to finish with some extra tests and documentation, and maybe also some extra methods.

Also not entirely sure about the "Keyset" name; but I couldn't think of anything better.

@Winnetoe24
Copy link
Author

i will look at it this weekend

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Winnetoe24 @arp242
0