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

Switch WorkPool of ConsumerWorkService to channels #882

Merged
merged 2 commits into from
Jul 1, 2020

Conversation

danielmarbach
Copy link
Collaborator

@danielmarbach danielmarbach commented Jun 25, 2020

Proposed Changes

Switches WorkPool of ConsumerWorkService to channels

As discussed with @stebet in conversation over here #866 (comment)

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

  • 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

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask on the
mailing list. We're here to help! This is simply a reminder of what we are
going to look for before merging your code.

  • 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

Further Comments

@stebet
Copy link
Contributor

stebet commented Jun 25, 2020

Looks good to me :)

}

static async Task OffloadToWorkerThreadPool(Action action, SemaphoreSlim limiter)
{
try
{
await Task.Run(() => action());
// like Task.Run but doesn't closure allocate
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉 I like =)

Copy link
Contributor

@sungam3r sungam3r Jul 11, 2020

Choose a reason for hiding this comment

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

Unfortunately, it still allocates delegate. IIRC it is well known issue of Roslyn. If you really do not want allocate delegate each call then define static delegate and use it here.
dotnet/roslyn#39869

Copy link
Member

Choose a reason for hiding this comment

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

@sungam3r thanks for chiming in. You are welcome to submit a PR that changes the comment (and links to the Roslyn issue) or even switched to use a static delegate if it's a safe thing to do in this context. Thank you.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Comparison according to sharplab

    [Serializable]
    [CompilerGenerated]
    private sealed class <>c
    {
        public static readonly <>c <>9 = new <>c();

        public static Action<object> <>9__0_0;

        internal void <TaskFactory>b__0_0(object state)
        {
            ((Action)state)();
        }
    }

awaiter = Task.Factory.StartNew(<>c.<>9__0_0 ?? (<>c.<>9__0_0 = new Action<object>(<>c.<>9.<TaskFactory>b__0_0)), action, CancellationToken.None, TaskCreationOptions.DenyChildAttach, TaskScheduler.Default).GetAwaiter();

so the Action allocation happens once and is then cached for further execution in a static Action field.

vs Task.Run(() => action())

                    <>c__DisplayClass1_0 <>c__DisplayClass1_ = new <>c__DisplayClass1_0();
                    <>c__DisplayClass1_.action = action;
                    awaiter = Task.Run(new Action(<>c__DisplayClass1_.<TaskRun>b__0)).GetAwaiter();

display class creation for every run plus action allocation for every run

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wrong. Such lambda usage is OK. I re-read the Roslyn issue and remembered details.

@lukebakken lukebakken added this to the 6.2.0 milestone Jul 1, 2020
@lukebakken lukebakken self-assigned this Jul 1, 2020
@lukebakken
Copy link
Contributor

Thanks everyone!

@lukebakken lukebakken merged commit c9eb9f1 into rabbitmq:master Jul 1, 2020
lukebakken added a commit that referenced this pull request Jul 1, 2020
Switch WorkPool of ConsumerWorkService to channels

(cherry picked from commit c9eb9f1)
@lukebakken
Copy link
Contributor

Backported to 6.x branch.

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.

6 participants