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

remove task yield #855

Merged
merged 1 commit into from
May 31, 2020
Merged

Conversation

bollhals
Copy link
Contributor

Proposed Changes

Removed the task.yield in the Work baseclass as well as the try catch, as there is already a try catch at the callee (Workpool of AsyncConsumerWorkService). This eliminates another chunk of allocations.

Types of Changes

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)

Checklist

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • All tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in related repositories

@bollhals
Copy link
Contributor Author

I don't see a reason for the task.yield.

From Testapp:
Before:
image
After:
image

=> 71 MB -> 65 MB

@michaelklishin
Copy link
Member

So an 8.5% improvement. This is certainly meaningful. I will trust your and @stebet's judgment on the need to yield.

@michaelklishin michaelklishin merged commit c395ca4 into rabbitmq:master May 31, 2020
@lukebakken
Copy link
Contributor

lukebakken commented May 31, 2020

@bollhals actually this has been discussed. Searching this repo for the string yield turns up this issue ...

#341

...and this PR...

#760

I will revert the changes here. We can't remove the yield until the client is refactored to be async, basically.

lukebakken added a commit that referenced this pull request May 31, 2020
This reverts commit c395ca4, reversing
changes made to ef01562.
@lukebakken
Copy link
Contributor

Reverted by aa5e8cb

@lukebakken lukebakken added the next-gen-todo If a rewrite happens, address this issue. label May 31, 2020
@bollhals
Copy link
Contributor Author

@bollhals actually this has been discussed. Searching this repo for the string yield turns up this issue ...

#341

...and this PR...

#760

I will revert the changes here. We can't remove the yield until the client is refactored to be async, basically.

I'd like to understand how this pervents the deadlock, but I haven't been able to find a test case to reproduce it. (I'd like to add one if there isn't. So it is clear to everyone why it is needed)
From the comments in #341 I wasn't able to break it, even with the code provided here.

@bollhals
Copy link
Contributor Author

bollhals commented May 31, 2020

Now that I think of it, Is it possible that #844 fixed the issue so it isn't needed anymore? @stebet

@lukebakken
Copy link
Contributor

This is something @danielmarbach and / or @bording should weigh in on if they have time. No hurry at all.

@danielmarbach
Copy link
Collaborator

I looked at the current implementation on master and I think the changes proposed here should be fine. If I recall correctly (and I doubt that given my bad memories) we basically introduced the yield as a precaution. Around the timeframe when the first version of the code was introduced we didn't really have a good possibility to run continuations async other than doing some nasty reflection tricks, or wrap the task completion source TrySet methods with Task.Run or enforce async continuations by doing the Task.Yield. With the current code either using RunContinuationsAsynchronously or setting the channels continuation behavior to AllowSynchronousContinuations = false this problem should be mitigated I think although I haven't spend a lot of time digging through everything.

@lukebakken lukebakken mentioned this pull request Jun 2, 2020
lukebakken pushed a commit that referenced this pull request Jun 12, 2020
lukebakken added a commit that referenced this pull request Jun 16, 2020
Re-merge pull request #855 from bollhals/remove.task.yield
lukebakken added a commit that referenced this pull request Jul 7, 2020
Re-merge pull request #855 from bollhals/remove.task.yield

(cherry picked from commit 7c9914a)
@bollhals bollhals deleted the remove.task.yield branch March 2, 2021 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next-gen-todo If a rewrite happens, address this issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants