Skip to content

Multi Get and Multi Set - #4

Open
Winnetoe24 wants to merge 1 commit into
arp242:v2from
Winnetoe24:multi-get-set
Open

Multi Get and Multi Set#4
Winnetoe24 wants to merge 1 commit into
arp242:v2from
Winnetoe24:multi-get-set

Conversation

@Winnetoe24

Copy link
Copy Markdown

No description provided.

@arp242

arp242 commented Dec 13, 2023

Copy link
Copy Markdown
Owner

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
Copy Markdown
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

arp242 commented Dec 14, 2023

Copy link
Copy Markdown
Owner

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

arp242 commented Dec 14, 2023

Copy link
Copy Markdown
Owner

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

Winnetoe24 commented Dec 14, 2023

Copy link
Copy Markdown
Author

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

arp242 commented Dec 14, 2023

Copy link
Copy Markdown
Owner

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
Copy Markdown
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
Copy Markdown
Author

I will further test tomorrow

@arp242

arp242 commented Dec 17, 2023

Copy link
Copy Markdown
Owner

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
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
@arp242

arp242 commented Jan 3, 2024

Copy link
Copy Markdown
Owner

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
Copy Markdown
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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants