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

Add KV try get #688

Merged
merged 9 commits into from
Dec 10, 2024
Merged

Add KV try get #688

merged 9 commits into from
Dec 10, 2024

Conversation

mtmk
Copy link
Collaborator

@mtmk mtmk commented Dec 4, 2024

KVBench.cs:

Method Mean Error StdDev Ratio RatioSD Gen0 Gen1 Allocated Alloc Ratio
TryGetAsync 150.3 us 2.87 us 2.95 us 0.89 0.02 0.2441 0.2441 4.5 KB 0.82
GetAsyncNew 170.5 us 3.33 us 3.56 us 1.01 0.03 0.2441 - 5.78 KB 1.05
GetAsync 168.3 us 2.82 us 2.50 us 1.00 0.02 0.2441 0.2441 5.49 KB 1.00

TryGet is performing much better.

the new Get (which is using TryGet) is slightly worst because of the additional async/await state machine I'm guessing.

cc @srs-adamr @to11mtm

Copy link

@srs-adamr srs-adamr left a comment

Choose a reason for hiding this comment

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

Looks good!

@mtmk mtmk requested a review from scottf December 5, 2024 18:48
@to11mtm
Copy link
Collaborator

to11mtm commented Dec 5, 2024

I'll give this a look tonight @mtmk

@to11mtm
Copy link
Collaborator

to11mtm commented Dec 5, 2024

the new Get (which is using TryGet) is slightly worst because of the additional async/await state machine I'm guessing.

Async await and Delegate state captures are very interesting. AFAIR for example, if you've got multiple anonymous delegates (e.x. myList.Where(a=> a.Foo == this.CaptureOne).Where(a=>a.Bar== captureBar).Where(a.FooBar == captureFooBar)) the compiler may prefer to re-use the same 'capture' type between instances (i.e. it's not re-using instances, but it keeps number of compiler generated types down), so your delegates can allocate more memory. In extreme cases, splitting synchronous logic out into separate calling methods was sometimes useful, both for this and to save on locals.

Copy link
Collaborator

@to11mtm to11mtm left a comment

Choose a reason for hiding this comment

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

I'm OK with it, My biggest concern would be my comments about EnsureSuccess and how to use, since that method would be exposed API.

The other stuff can be handled via future PR if needed, but hopefully is good food for thought for possible perf/mem improvements

src/NATS.Client.Core/NatsResult.cs Outdated Show resolved Hide resolved
src/NATS.Client.KeyValueStore/NatsKVStore.cs Outdated Show resolved Hide resolved
src/NATS.Client.KeyValueStore/NatsKVStore.cs Outdated Show resolved Hide resolved
src/NATS.Client.KeyValueStore/NatsKVStore.cs Show resolved Hide resolved
Copy link
Collaborator

@to11mtm to11mtm left a comment

Choose a reason for hiding this comment

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

More comments

src/NATS.Client.KeyValueStore/NatsKVStore.cs Outdated Show resolved Hide resolved
src/NATS.Client.KeyValueStore/NatsKVStore.cs Outdated Show resolved Hide resolved
@mtmk mtmk self-assigned this Dec 7, 2024
Copy link
Collaborator

@to11mtm to11mtm left a comment

Choose a reason for hiding this comment

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

Reviewed general cleanup, looks awesome!

src/NATS.Client.KeyValueStore/NatsKVStore.cs Show resolved Hide resolved
src/NATS.Client.KeyValueStore/NatsKVStore.cs Show resolved Hide resolved
Copy link
Contributor

@scottf scottf left a comment

Choose a reason for hiding this comment

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

LGTM

@mtmk mtmk merged commit 150d5a8 into main Dec 10, 2024
13 checks passed
@mtmk mtmk deleted the kv-try-get branch December 10, 2024 08:24
mtmk added a commit that referenced this pull request Dec 10, 2024
* Add password support for PFX files (#694)
* Add KV try get (#688)
* Jetstream CreateOrUpdateStream (#692)
* Update README.md (#683)
* Add (force) reconnect method (#684)
* Add flags to NatsMsg and JetStream Request (#652)
@mtmk mtmk mentioned this pull request Dec 10, 2024
mtmk added a commit that referenced this pull request Dec 10, 2024
* Add password support for PFX files (#694)
* Add KV try get (#688)
* Jetstream CreateOrUpdateStream (#692)
* Update README.md (#683)
* Add (force) reconnect method (#684)
* Add flags to NatsMsg and JetStream Request (#652)
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.

4 participants