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

Create benchmarks for System.Threading.Tasks.Dataflow #951

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

manandre
Copy link
Contributor

I propose multiple benchmarks for the System.Threading.Tasks.Dataflow library.
They cover some main performance-related use cases for each type of Dataflow block (BufferBlock, ActionBlock, TransformBlock…)

Fixes #950

@dnfclas
Copy link

dnfclas commented Oct 17, 2019

CLA assistant check
All CLA requirements met.

Refactoring is not always your friend...
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

Hello @manandre

Thank you for your contribution!

This PR is non-trivial, it's going to take me a while to review it properly.

To make it easier to read, could you please do the following:

  • reduce the inheritance hierarchy depth
  • intoduce the classes in order they are referenced in the code: example: if class 'B' derives from 'A', then 'A' should be the first class in a file
  • try to devide the #regions into separate C# files

Thank you,
Adam

@manandre manandre requested a review from adamsitnik October 22, 2019 08:42
@manandre
Copy link
Contributor Author

manandre commented Nov 4, 2019

@adamsitnik Are there so many issues? 😄
Do you already have some first remarks to share?

@adamsitnik
Copy link
Member

@stephentoub could you please take a look?

@adamsitnik
Copy link
Member

Are there so many issues?

@manandre no, appologies for the delay. It looks good to me from the benchmarking perspective, but I would like to know @stephentoub opinion before we merge it

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Thanks for writing these.

I'm a little worried that some of these tests aren't testing what they're meant to be testing, both because a) of how GlobalSetups work in relation to the tests being invoked and state changes those tests make, and b) the overheads involved in the tests themselves, e.g. using LINQ queries, using Task.Run, etc. I'm also finding the structure here a bit hard to follow, making it harder to see for sure whether there's actually an issue.

public void BlockSetup()
{
block = CreateBlock();
}
Copy link
Member

Choose a reason for hiding this comment

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

Why is this GlobalSetup still needed when we're calling BlockSetup on each benchmark invocation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only call BlockSetup from the benchmark code when we are forced to call Complete on the tested block to have a valid test case. This concerns only two benchmarks.

@stephentoub
Copy link
Member

At a high-level this seems fine. Personally I think the abstraction is unnecessary and detracts rather than adds value, but I'll leave that up to Adam. I'm also a little skeptical about the setup because I suspect many of the tests are going to be measuring costs that aren't actually that relevant, e.g. a given test here creates a block, does stuff, and then waits for the block to complete, and that's going to include the overhead of creating the blocks, which in many cases would be done much less frequently. Regardless, thanks for writing these and contributing.

@manandre
Copy link
Contributor Author

manandre commented Dec 7, 2019

@adamsitnik Do we need to go further ? If yes, please indicate which points to address 🎯

Base automatically changed from master to main March 18, 2021 17:12
@billwert
Copy link
Member

@adamsitnik Should this be merged?

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.

Dataflow needs performance tests
5 participants