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.

Sign up for GitHub

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

Jump to bottom

Circular Buffer: added memory leak tests for overwrite and read #1652

Closed
wants to merge 1 commit into from
Closed

Circular Buffer: added memory leak tests for overwrite and read #1652

wants to merge 1 commit into from

Conversation

felix91gr
Copy link
Contributor

When solving this with unsafe, my mentor pointed out that my overwrite method must've been leaking memory. He was correct! And I fixed this issue in my next iteration.

A few days ago however, I remembered that there was a test for memory leaks on the clear method. The test in question uses Rc and its strong count to check if memory has been freed or not from the buffer.

I decided to base a new couple of tests on the same technique, and well, indeed my earlier buffer fails the overwrite test.

I've added these two to make sure the test suite helps students implement a leak-free Buffer :)

@github-actions
Copy link
Contributor

Hello. Thanks for opening a PR on Exercism. We are currently in a phase of our journey where we have paused community contributions to allow us to take a breather and redesign our community model. You can learn more in this blog post. As such, all issues and PRs in this repository are being automatically closed.

That doesn't mean we're not interested in your ideas, or that if you're stuck on something we don't want to help. The best place to discuss things is with our community on the Exercism Community Forum. You can use this link&category=rust) to copy this into a new topic there.

Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

@github-actions github-actions bot closed this Mar 17, 2023
@senekor senekor reopened this Jul 18, 2023
@senekor
Copy link
Contributor

senekor commented Jul 18, 2023

Hah. The tests fail on the example solution.

On the one hand, I'm not sure if the Rc approach is completely bullet proof for detecting memory leaks. But it seems very pragmatic though, so I'm in favor.

The example solution should be improved, using Default + Clone to avoid having to use unsafe is not a correct approach in my opinion. It can be adjusted slightly to only rely on Default and pass the additional tests. That's better, but requiring Default is still not optimal. Other approaches:

  • use Vec<Option<T>> internally, safe and correct but less memory efficient. (my personal solution does this)
  • unsafe. I'd be hesitant to use it in example solutions.
  • (check community solutions for more approaches)

senekor added a commit that referenced this pull request Sep 10, 2023
@senekor
Improve example solution of circular-buffer
5df3917 
The trait bounds `Default + Clone` are semantically incorrect.
A proper circular buffer needs to handle all kinds of elements.

The reason these bounds are here is because it allows to avoid `unsafe`
while enjoying the most efficient memory layout.
However, there is another performance downside:
The implementations of `Default` and `Clone` may be expensive
(e.g. cause heap allocations.)

As came up in this discussion, there are other weird side effects,
for example for `Rc` which has a non-standard implementation of `Clone`:
#1652

The approach I chose here get's rid of the trait bounds and wraps
every element in an `Option`.
In the worst case, this may lead to twice the memory consumption.
(e.g. `Option<u64>` takes 16 bytes because of alignment)
This approach is semantically correct, but not the most performant.

The most performant solution would be to use `unsafe`.
I'd like to avoid that in example solutions.
@senekor senekor mentioned this pull request Sep 10, 2023
Merged
@senekor
Copy link
Contributor

senekor commented Sep 10, 2023

I created a PR to improve the example solution.

But I don't think the tests should be added anymore. The exercise is present in the problem-specifications repository, which means new tests should be added there.

@senekor senekor closed this Sep 10, 2023
senekor added a commit that referenced this pull request Sep 11, 2023
@senekor
Improve example solution of circular-buffer
40dc162 
The trait bounds `Default + Clone` are semantically incorrect.
A proper circular buffer needs to handle all kinds of elements.

The reason these bounds are here is because it allows to avoid `unsafe`
while enjoying the most efficient memory layout.
However, there is another performance downside:
The implementations of `Default` and `Clone` may be expensive
(e.g. cause heap allocations.)

As came up in this discussion, there are other weird side effects,
for example for `Rc` which has a non-standard implementation of `Clone`:
#1652

The approach I chose here get's rid of the trait bounds and wraps
every element in an `Option`.
In the worst case, this may lead to twice the memory consumption.
(e.g. `Option<u64>` takes 16 bytes because of alignment)
This approach is semantically correct, but not the most performant.

The most performant solution would be to use `unsafe`.
I'd like to avoid that in example solutions.
senekor added a commit that referenced this pull request Sep 11, 2023
@senekor
Improve example solution of circular-buffer
c406b9c 
The trait bounds `Default + Clone` are semantically incorrect.
A proper circular buffer needs to handle all kinds of elements.

The reason these bounds are here is because it allows to avoid `unsafe`
while enjoying the most efficient memory layout.
However, there is another performance downside:
The implementations of `Default` and `Clone` may be expensive
(e.g. cause heap allocations.)

As came up in this discussion, there are other weird side effects,
for example for `Rc` which has a non-standard implementation of `Clone`:
#1652

The approach I chose here get's rid of the trait bounds and wraps
every element in an `Option`.
In the worst case, this may lead to twice the memory consumption.
(e.g. `Option<u64>` takes 16 bytes because of alignment)
This approach is semantically correct, but not the most performant.

The most performant solution would be to use `unsafe`.
I'd like to avoid that in example solutions.
senekor added a commit that referenced this pull request Sep 11, 2023
@senekor
Improve example solution of circular-buffer
df1742b 
The trait bounds `Default + Clone` are semantically incorrect.
A proper circular buffer needs to handle all kinds of elements.

The reason these bounds are here is because it allows to avoid `unsafe`
while enjoying the most efficient memory layout.
However, there is another performance downside:
The implementations of `Default` and `Clone` may be expensive
(e.g. cause heap allocations.)

As came up in this discussion, there are other weird side effects,
for example for `Rc` which has a non-standard implementation of `Clone`:
#1652

The approach I chose here get's rid of the trait bounds and wraps
every element in an `Option`.
In the worst case, this may lead to twice the memory consumption.
(e.g. `Option<u64>` takes 16 bytes because of alignment)
This approach is semantically correct, but not the most performant.

The most performant solution would be to use `unsafe`.
I'd like to avoid that in example solutions.
senekor added a commit that referenced this pull request Sep 12, 2023
@senekor
Improve example solution of circular-buffer
d1e235c 
The trait bounds `Default + Clone` are semantically incorrect.
A proper circular buffer needs to handle all kinds of elements.

The reason these bounds are here is because it allows to avoid `unsafe`
while enjoying the most efficient memory layout.
However, there is another performance downside:
The implementations of `Default` and `Clone` may be expensive
(e.g. cause heap allocations.)

As came up in this discussion, there are other weird side effects,
for example for `Rc` which has a non-standard implementation of `Clone`:
#1652

The approach I chose here get's rid of the trait bounds and wraps
every element in an `Option`.
In the worst case, this may lead to twice the memory consumption.
(e.g. `Option<u64>` takes 16 bytes because of alignment)
This approach is semantically correct, but not the most performant.

The most performant solution would be to use `unsafe`.
I'd like to avoid that in example solutions.
senekor added a commit that referenced this pull request Sep 12, 2023
@senekor
Improve example solution of circular-buffer
9e4c8c5 
The trait bounds `Default + Clone` are semantically incorrect.
A proper circular buffer needs to handle all kinds of elements.

The reason these bounds are here is because it allows to avoid `unsafe`
while enjoying the most efficient memory layout.
However, there is another performance downside:
The implementations of `Default` and `Clone` may be expensive
(e.g. cause heap allocations.)

As came up in this discussion, there are other weird side effects,
for example for `Rc` which has a non-standard implementation of `Clone`:
#1652

The approach I chose here get's rid of the trait bounds and wraps
every element in an `Option`.
In the worst case, this may lead to twice the memory consumption.
(e.g. `Option<u64>` takes 16 bytes because of alignment)
This approach is semantically correct, but not the most performant.

The most performant solution would be to use `unsafe`.
I'd like to avoid that in example solutions.
senekor added a commit that referenced this pull request Sep 12, 2023
@senekor
Improve example solution of circular-buffer
746cfd5 
The trait bounds `Default + Clone` are semantically incorrect.
A proper circular buffer needs to handle all kinds of elements.

The reason these bounds are here is because it allows to avoid `unsafe`
while enjoying the most efficient memory layout.
However, there is another performance downside:
The implementations of `Default` and `Clone` may be expensive
(e.g. cause heap allocations.)

As came up in this discussion, there are other weird side effects,
for example for `Rc` which has a non-standard implementation of `Clone`:
#1652

The approach I chose here get's rid of the trait bounds and wraps
every element in an `Option`.
In the worst case, this may lead to twice the memory consumption.
(e.g. `Option<u64>` takes 16 bytes because of alignment)
This approach is semantically correct, but not the most performant.

The most performant solution would be to use `unsafe`.
I'd like to avoid that in example solutions.
senekor added a commit that referenced this pull request Sep 13, 2023
@senekor
Improve example solution of circular-buffer
b388172 
The trait bounds `Default + Clone` are semantically incorrect.
A proper circular buffer needs to handle all kinds of elements.

The reason these bounds are here is because it allows to avoid `unsafe`
while enjoying the most efficient memory layout.
However, there is another performance downside:
The implementations of `Default` and `Clone` may be expensive
(e.g. cause heap allocations.)

As came up in this discussion, there are other weird side effects,
for example for `Rc` which has a non-standard implementation of `Clone`:
#1652

The approach I chose here get's rid of the trait bounds and wraps
every element in an `Option`.
In the worst case, this may lead to twice the memory consumption.
(e.g. `Option<u64>` takes 16 bytes because of alignment)
This approach is semantically correct, but not the most performant.

The most performant solution would be to use `unsafe`.
I'd like to avoid that in example solutions.
senekor added a commit that referenced this pull request Sep 14, 2023
@senekor
Improve example solution of circular-buffer
d3ed68a 
The trait bounds `Default + Clone` are semantically incorrect.
A proper circular buffer needs to handle all kinds of elements.

The reason these bounds are here is because it allows to avoid `unsafe`
while enjoying the most efficient memory layout.
However, there is another performance downside:
The implementations of `Default` and `Clone` may be expensive
(e.g. cause heap allocations.)

As came up in this discussion, there are other weird side effects,
for example for `Rc` which has a non-standard implementation of `Clone`:
#1652

The approach I chose here get's rid of the trait bounds and wraps
every element in an `Option`.
In the worst case, this may lead to twice the memory consumption.
(e.g. `Option<u64>` takes 16 bytes because of alignment)
This approach is semantically correct, but not the most performant.

The most performant solution would be to use `unsafe`.
I'd like to avoid that in example solutions.
senekor added a commit that referenced this pull request Sep 15, 2023
@senekor
Improve example solution of circular-buffer (#1724)
2a14ac5 
The trait bounds `Default + Clone` are semantically incorrect.
A proper circular buffer needs to handle all kinds of elements.

The reason these bounds are here is because it allows to avoid `unsafe`
while enjoying the most efficient memory layout.
However, there is another performance downside:
The implementations of `Default` and `Clone` may be expensive
(e.g. cause heap allocations.)

As came up in this discussion, there are other weird side effects,
for example for `Rc` which has a non-standard implementation of `Clone`:
#1652

The approach I chose here get's rid of the trait bounds and wraps
every element in an `Option`.
In the worst case, this may lead to twice the memory consumption.
(e.g. `Option<u64>` takes 16 bytes because of alignment)
This approach is semantically correct, but not the most performant.

The most performant solution would be to use `unsafe`.
I'd like to avoid that in example solutions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@felix91gr @senekor