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

Add stress fd test #763

Closed

Conversation

RichardChukwu
Copy link

@RichardChukwu RichardChukwu commented Oct 4, 2024

Title: Add Stress Test for Open File Descriptor (FD) Leaks

Description:

This pull request adds a new stress test (stress_fd.ml) to check for file descriptor (FD) leaks in Eio. The test ensures that the number of open FDs at the beginning of the test matches the count at the end, helping to identify resource leaks under high load.

Test Details:

Test Objective: Validate that Eio properly handles file descriptors during concurrent domain and fiber operations without leaks.
Approach:

The test creates multiple domains and fibers, which perform various IO operations (e.g., reading/writing to a file).
At the end of the test, the number of open file descriptors is checked to ensure no leaks occurred.

Rationale: This addresses a common source of performance degradation and resource exhaustion when managing IO operations in parallel.

Benchmark Improvements:

This test contributes to the ongoing effort to improve benchmarking and stress testing of Eio, particularly in areas related to OS integration, as discussed in issue #450.

Related Issues:
related to #450: Add more stress tests and benchmarks

@RichardChukwu
Copy link
Author

Hello @patricoferris I'd appreciate your feedback on this, thank you

Copy link
Collaborator

@patricoferris patricoferris left a comment

Choose a reason for hiding this comment

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

Before reviewing the code, let's get the stress test building :))

The dune project file isn't doing anything. I think reading the dune quickstart will help see the differences between dune-project and dune files.

Additionally, have a go at running some of the stress tests yourself, e.g.

dune exec -- stress/stress_semaphore.exe

Then have a look to see how these are being built by dune, so you can add your test there too.

@talex5
Copy link
Collaborator

talex5 commented Oct 5, 2024

This looks AI-generated to me. The code doesn't match the description. Also, #762 (comment) looks AI-generated to me, though I'm not very familiar with this stuff.

@patricoferris do we need to add some rule somewhere about this? I see https://kernelnewbies.org/Outreachyfirstpatch says "Contributions that include content generated by AI tools that may rely on sources that are not compatible with GPLv2 thus cannot be accepted into the Linux kernel. This includes ChatGPT." Maybe we need something similar for Eio?

@RichardChukwu
Copy link
Author

Does it mean I can't work on this until issue #761 is resolved please? @patricoferris

@talex5 I looked at the other stress tests and wrote the code in the same manner, please I'd like to know if that applies?

@patricoferris
Copy link
Collaborator

Does it mean I can't work on this until issue #761 is resolved please?

Yes, that's right. Let's keep our focus on a single issue and PR.

Contributions that include content generated by AI tools that may rely on sources that are not compatible with GPLv2 thus cannot be accepted into the Linux kernel. This includes ChatGPT.

I think we should have a rule, though I'm not sure what it is. I'm not a lawyer, and from the very light searching I did, it would seem that the licensing issue exists in a pretty grey area. However, that's not to say we can't have our own rule. I'll draft something for the CONTRIBUTING.md maybe?

@RichardChukwu -- we really appreciate you taking the time to engage and contribute to Eio. To be clear, I don't think what we're saying here is that contributions can't use AI. But it should be used as a tool to help write a PR, not as a tool to write the whole PR, your comments and PR description for you. There are a few reasons for this.

  1. Most importantly, it obfuscates for us how you think. Without being able to see how you think, it makes it much harder for us to help you learn and subsequently to land the PR.
  2. It is more work for us. This is related to (1), but reviewing code generated by AI at this point is inefficient and unproductive. We could have piped the issue into ChatGPT (or similar) ourselves, but the point of Outreachy is to help you contribute to open-source.

So I think we can maybe try this PR again, with your thoughts, descriptions, code and maybe even bugs :)) ? In which case, I'm going to close this PR. Looking forward to seeing a new one when you have the time.

@RichardChukwu
Copy link
Author

Duly noted @patricoferris thank you so much for this feedback, I understand you perfectly and will take corrections accordingly.

Let me figure out another issue to work on then. How about assigning #761 to me first, since that's needed for this isuse to be worked on and is more related to Windows?

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.

3 participants