Skip to content

Main #7

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

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

Main #7

wants to merge 3 commits into from

Conversation

Gkodkod
Copy link

@Gkodkod Gkodkod commented Dec 26, 2024

No description provided.

Comment on lines +113 to +126
var results = new ConcurrentBag<TaskMetrics>();

// Parallel task execution with improved task management
var tasks = Enumerable.Range(0, numTasks)
.Select(taskId => Task.Run(async () =>
{
var result = await PerformTaskAsync(taskId, logger);
results.Add(result);
return result;
}))
.ToList();

// Efficient task completion tracking
await Task.WhenAll(tasks);
Copy link
Owner

Choose a reason for hiding this comment

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

You are capturing locals in the closure and creating unnecessary delegates.
Also, unlike Rust, by default .NET schedules async Tasks and yields immediately, so you don't need to wrap it in a Task.Run at all (which would result in creating two Tasks for every single async Task). And the usage to ConcurrentBag is unnecessary.
The code should be simple to avoid introducing extra overhead which is unrelated to the concurrency structure itself.

Suggested change
var results = new ConcurrentBag<TaskMetrics>();
// Parallel task execution with improved task management
var tasks = Enumerable.Range(0, numTasks)
.Select(taskId => Task.Run(async () =>
{
var result = await PerformTaskAsync(taskId, logger);
results.Add(result);
return result;
}))
.ToList();
// Efficient task completion tracking
await Task.WhenAll(tasks);
var tasks = new List<Task<TaskMetrics>>(numTasks);
for (int i = 0; i < numTasks; i++)
{
tasks.Add(PerformTaskAsync(i, logger));
}
await Task.WhenAll(results); // this will gives you TaskMetrics[] which contains all the results

public static long GetMemoryUsage()
{
// Leverage .NET 9 cross-platform memory tracking
return GC.GetTotalMemory(false);
Copy link
Owner

Choose a reason for hiding this comment

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

We should rely on external tools to measure the memory. GC.GetTotalMemory only accounts for managed memory which doesn't include the runtime overhead.

Comment on lines -8 to +11
<InvariantGlobalization>true</InvariantGlobalization>

<!-- .NET 9 specific optimizations -->
<PublishTrimmed>true</PublishTrimmed>
<PublishAot>true</PublishAot>
<TrimmerDefaultAction>link</TrimmerDefaultAction>
Copy link
Owner

Choose a reason for hiding this comment

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

We don't need the globalization here, which aligns with the behavior of other languages. Otherwise, it would lead the runtime to load ICU globalization data into the memory.
Also, when PublishAot is true, the trimmer behavior is implied so you don't need to set PublishTrimmed or TrimmerDefaultAction at all.

Comment on lines +16 to +18
<PackageReference Include="System.Diagnostics.DiagnosticSource" Version="9.0.0-preview.1.*" />
<PackageReference Include="Microsoft.Extensions.Logging" Version="9.0.0-preview.1.*" />
<PackageReference Include="Microsoft.Extensions.Logging.Console" Version="9.0.0-preview.1.*" />
Copy link
Owner

@hez2010 hez2010 Dec 26, 2024

Choose a reason for hiding this comment

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

Why are you using preview versions while the 9.0 GA versions are present?
And I don't see why a logger is needed when you can just use Console.WriteLine.

Comment on lines +24 to +28
func getMemoryStats() uint64 {
var m runtime.MemStats
runtime.ReadMemStats(&m)
return m.Alloc
}
Copy link
Owner

Choose a reason for hiding this comment

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

Same with above. We should rely on external tools to measure the memory.

Comment on lines +15 to +16
const memoryUsage = process.memoryUsage();
const v8HeapStats = v8.getHeapStatistics();
Copy link
Owner

Choose a reason for hiding this comment

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

Same with above. We should rely on external tools to measure the memory.

await asyncio.sleep(10)

end_time = time.time()
end_memory = memory_profiler.memory_usage()[0]
Copy link
Owner

Choose a reason for hiding this comment

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

Same with above. We should rely on external tools to measure the memory.

Comment on lines +46 to +47
self.memory_start = tracemalloc.get_traced_memory()[0]
self.memory_end = None
Copy link
Owner

Choose a reason for hiding this comment

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

Same with above. We should rely on external tools to measure the memory.

Comment on lines +45 to +58
unsafe impl<A: std::alloc::GlobalAlloc> std::alloc::GlobalAlloc for MemoryTrackingAllocator<A> {
unsafe fn alloc(&self, layout: std::alloc::Layout) -> *mut u8 {
let ptr = self.allocator.alloc(layout);
if !ptr.is_null() {
GLOBAL_MEMORY_TRACKER.fetch_add(layout.size(), Ordering::Relaxed);
}
ptr
}

unsafe fn dealloc(&self, ptr: *mut u8, layout: std::alloc::Layout) {
self.allocator.dealloc(ptr, layout);
GLOBAL_MEMORY_TRACKER.fetch_sub(layout.size(), Ordering::Relaxed);
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Same with above. We should rely on external tools to measure the memory.


async fn perform_task(task_id: usize, allocator: &TracingAllocator<std::alloc::System>) -> TaskResult {
let start_time = Instant::now();
let start_memory = allocator.allocated.load(Ordering::Relaxed);
Copy link
Owner

Choose a reason for hiding this comment

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

Same with above. We should rely on external tools to measure the memory.

@Gkodkod
Copy link
Author

Gkodkod commented Dec 26, 2024

@hez2010 Hi Steve, You are comparing apples to oranges.

Thank you for taking the time to review the PRs and provide your comments. I appreciate the effort you’ve put into creating and sharing this benchmark. However, I wanted to offer some constructive feedback regarding the methodology and comparison criteria used.

In particular, I noticed that the benchmark compares different concurrency models (e.g., Node.js promises versus Go goroutines, and an absult disregard to other languages), which operate under fundamentally different paradigms. This approach can lead to comparisons that are not entirely equitable or reflective of real-world scenarios.

I believe that aligning the benchmarking methodologies with the unique characteristics and strengths of each language and runtime would result in a more balanced and accurate assessment. Additionally, considerations such as compiler optimizations, memory consumption patterns, and workload diversity are critical to producing meaningful benchmarks.

It's worth noting that some discussions, such as a Hacker News thread, have critiqued the methodologies used in your benchmark favoring .NET, without mention of C, C++, Rust and Java's superiority in high-letanacy production systems.

https://news.ycombinator.com/item?id=42270378

Think this over after listening to our advice. Happy New Years !

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.

2 participants