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

Request optimizations #339

Merged
merged 10 commits into from
Nov 26, 2019
Merged

Request optimizations #339

merged 10 commits into from
Nov 26, 2019

Conversation

danielwertheim
Copy link
Collaborator

@danielwertheim danielwertheim commented Nov 20, 2019

After these changes when running the Benchmarks included I see an increase with about 20% in the request async flow.

When debugging and monitoring GC activity (as discussed in #299), both with and without a specified timeout, I see a highly reduced GC activity also leading to reducing CPU activity.

@danielwertheim danielwertheim self-assigned this Nov 20, 2019
@danielwertheim danielwertheim changed the title Async request optimizations Request optimizations Nov 20, 2019
@danielwertheim
Copy link
Collaborator Author

@ColinSullivan1 having the global subscription created immediately when connected, is that avoided due to not creating an unnecessary subscription? Could this be OK if we added config for the behavior? E.g options.LazyGlobalSubscriptionInit = true

@danielwertheim
Copy link
Collaborator Author

Reverted a commit. As it's not possible to specify both timeout and cancellationToken in the public request API, I thought there was no need for a CancellationTokenSource when a Token had been specified but no timeOut had been specified. But it is. Otherwise it never times out and we get a deadlock. Will look into that further and potentially tweak in future PR.

Copy link
Collaborator

@watfordgnf watfordgnf left a comment

Choose a reason for hiding this comment

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

Benchmarks on my machine are certainly faster. Looks good!

src/Samples/WinFormsSample/Form1.cs Outdated Show resolved Hide resolved
src/Samples/WinFormsSample/Form1.cs Outdated Show resolved Hide resolved
src/NATS.Client/Conn.cs Show resolved Hide resolved
src/NATS.Client/Conn.cs Show resolved Hide resolved
@watfordgnf
Copy link
Collaborator

In my app's case, we have maybe only one or two request-reply usages. All the rest are publish. We use web services for request/reply (and authorization). In our case, lazily creating the global subscription would be our preference.

Is there a performance gain by creating it immediately for heavy request/reply scenarios?

@danielwertheim
Copy link
Collaborator Author

Possibly this will solve #202 as well

Copy link
Member

@ColinSullivan1 ColinSullivan1 left a comment

Choose a reason for hiding this comment

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

One question about int vs string for the request id, and there's one check that I think needs to be moved under a lock. Other than that, LGTM.

@danielwertheim
Copy link
Collaborator Author

@ColinSullivan1 For ease, removing interlocked etc. a Guid would be easy. But I guess you don't want a longer subscription etc. than necessary

@danielwertheim danielwertheim merged commit 5254654 into nats-io:master Nov 26, 2019
@danielwertheim danielwertheim deleted the issue-299-asyncreqopt branch November 26, 2019 19:08
@ColinSullivan1
Copy link
Member

@danielwertheim , GUID would certainly work, but imo this will be faster and because the first token of the global inbox subject is a random set of bytes seeded by a GUI, we should be safe the way it is, and it'll reduce pressure at scale (smaller subjects as you mentioned).

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.

3 participants