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

Significantly improve .NET's code #8

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

Conversation

OoLunar
Copy link

@OoLunar OoLunar commented May 28, 2023

Significantly improves .NET's code, closes #4

Additionally ValueTasks are typically used in hotpaths over Task's due to their newer implementation and don't suffer the scars of .NET framework.

- Updated to .NET 7
- Dropped `ImplicitUsings` in dotnet.csproj
- Uses an array of Task[] over a list of List<Task> to prevent constant resizing of the internally backed array.
- Dropped Task.Run and passes the Task itself in the for loop.
@batzen
Copy link

batzen commented May 29, 2023

You added bin and obj folders.
I don't think they should be included in git. 😉

@OoLunar
Copy link
Author

OoLunar commented May 29, 2023

Whoops! You're right

@heku
Copy link

heku commented Jun 4, 2023

Good job @OoLunar , I agree your correcations.

@rolandcheck
Copy link

in fact would be better to use top-level statements to show beauty of modern c# for the doubters hehe
also i'd use Task WhenAll(IEnumerable<Task> tasks) overload so i dont allocate array by myself

my code would look like this:

const int delayMs = 1000 * 10; 
var taskCount = args.Length > 0 ? int.Parse(args[0]) : 100000;
var tasks = Enumerable
    .Range(0, taskCount)
    .Select(_ => Task.Delay(delayMs));
await Task.WhenAll(tasks);

@leo-costa
Copy link

in fact would be better to use top-level statements to show beauty of modern c# for the doubters hehe also i'd use Task WhenAll(IEnumerable<Task> tasks) overload so i dont allocate array by myself

my code would look like this:

const int delayMs = 1000 * 10; 
var taskCount = args.Length > 0 ? int.Parse(args[0]) : 100000;
var tasks = Enumerable
    .Range(0, taskCount)
    .Select(_ => Task.Delay(delayMs));
await Task.WhenAll(tasks);

And also do top level namespace

@OoLunar
Copy link
Author

OoLunar commented Jun 11, 2023

Have either one of y'all benchmarked your new code? While I agree, the code could look appealing to non-C# users, the goal here is to provide efficiency, not readability.

i'd use Task WhenAll(IEnumerable tasks) overload so i dont allocate array by myself

What... do you think an enumerable is, and how do you think it is internally represented? Arrays are quite efficient upon iteration, indexing and long lifetimes. Enumerables are a read-only interface which hides implementation from user. The goal here is to create X amount of tasks and wait for all of them to finish executing, thus there is no reason to hide the implementation via enumerables since we are the only people who are able to access the implementation.

@rolandcheck
Copy link

@OoLunar of course i did benchmark different solutions, as far as i remember there was no noticeable difference between your and my code, but original code was somewhat inefficient.
I pretty much know what Enumerable is, dont tell me that.
by saying use overload i do not "hide the implementation", i use existing overload of Task.WhenAll method. also what implementation did i hide? filling an array? isnt it just your implementation?

@akiraveliara
Copy link

There's no noticeable difference because the bulk of the benchmark is waitinh on the tasks to complete, however, in a less contrived and more realistic scenario, LINQ vs manual construction can make a huge difference - and if your goal is to showcase efficient C# code, using an infamous performance hog (LINQ) doesn't accomplish that.

@rolandcheck
Copy link

@akiraveliara my goal is to create readable & efficient code for this particular problem. i know linq can be expensive in some cases, but it does not bother me as it really does not matter in this context

@OoLunar
Copy link
Author

OoLunar commented Jun 11, 2023

This particular problem does not require readable code, it requires efficient code. You are welcome to create a separate pull request if you believe otherwise.

@JulianusIV
Copy link

@akiraveliara my goal is to create readable & efficient code for this particular problem. i know linq can be expensive in some cases, but it does not bother me as it really does not matter in this context

if your goal is to write both readable and efficient code you failed in both departments
LINQ is simply not as intuitive - especially to people new to c# - as just using a plain old array, and due to the abstractions that are hidden behind the scenes when using IEnumerable you take a hit in performance. You wont get more efficient than just using an array. Even if it is just a few nanoseconds, this is supposed to be a benchmark, so even that magnitude matters.

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.

7 participants