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

fix: chunk memory leak, bug fix #491

Merged
merged 1 commit into from
Jul 15, 2024
Merged

Conversation

mihir20
Copy link
Contributor

@mihir20 mihir20 commented Jul 15, 2024

raising a fix as mentioned in: #438

Copy link
Contributor

@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@@ -182,7 +182,7 @@ func Chunk[T any, Slice ~[]T](collection Slice, size int) []Slice {
if last > len(collection) {
last = len(collection)
}
result = append(result, collection[i*size:last])
result = append(result, collection[i*size:last:last])
Copy link
Contributor

Choose a reason for hiding this comment

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

I had never faced this notation, with the capacity apparently

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +239 to +243
// appending to a chunk should not affect original array
originalArray := []int{0, 1, 2, 3, 4, 5}
result5 := Chunk(originalArray, 2)
result5[0] = append(result5[0], 6)
is.Equal(originalArray, []int{0, 1, 2, 3, 4, 5})
Copy link
Contributor

Choose a reason for hiding this comment

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

So. it's not only a fix of a possible memory leak, but also a functional fix, no?

I'm unsure why you are adding this check.no computer with me to check.

My point is that the commit message could mention a memory leak fix + bug fix, maybe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, updated the title

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks

@mihir20 mihir20 changed the title fix: chunk memory leak fix: chunk memory leak, bug fix Jul 15, 2024
@samber
Copy link
Owner

samber commented Jul 15, 2024

I would recommend using copy() instead of a pointer to the same memory space. I think this is the spirit of this library. WDYT?

It consumes more memory, but it seems safer to me.

@mihir20
Copy link
Contributor Author

mihir20 commented Jul 15, 2024

I would recommend using copy() instead of a pointer to the same memory space. I think this is the spirit of this library. WDYT?

It consumes more memory, but it seems safer to me.

using copy will be slower compared to current implementation:
Benchmark using copy:

BenchmarkChunk
BenchmarkChunk/strings_10
BenchmarkChunk/strings_10-12         	14886344	        80.03 ns/op
BenchmarkChunk/strings_100
BenchmarkChunk/strings_100-12        	 1814036	       660.8 ns/op
BenchmarkChunk/strings_1000
BenchmarkChunk/strings_1000-12       	  183382	      6143 ns/op
BenchmarkChunk/ints10
BenchmarkChunk/ints10-12             	20346310	        57.41 ns/op
BenchmarkChunk/ints100
BenchmarkChunk/ints100-12            	 2672658	       433.6 ns/op
BenchmarkChunk/ints1000
BenchmarkChunk/ints1000-12           	  312517	      3871 ns/op

Benchmark without using copy:

BenchmarkChunk
BenchmarkChunk/strings_10
BenchmarkChunk/strings_10-12         	52708929	        22.42 ns/op
BenchmarkChunk/strings_100
BenchmarkChunk/strings_100-12        	12681414	        93.45 ns/op
BenchmarkChunk/strings_1000
BenchmarkChunk/strings_1000-12       	 2022960	       599.6 ns/op
BenchmarkChunk/ints10
BenchmarkChunk/ints10-12             	52086441	        22.50 ns/op
BenchmarkChunk/ints100
BenchmarkChunk/ints100-12            	12925526	        90.06 ns/op
BenchmarkChunk/ints1000
BenchmarkChunk/ints1000-12           	 1978053	       591.1 ns/op

@samber
Copy link
Owner

samber commented Jul 15, 2024

I will discuss it in another issue.

@samber samber mentioned this pull request Jul 15, 2024
@samber samber merged commit 9e34397 into samber:master Jul 15, 2024
7 checks passed
github-actions bot referenced this pull request in kairos-io/provider-kairos Jul 15, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [github.com/samber/lo](https://togithub.com/samber/lo) | `v1.45.0` ->
`v1.46.0` |
[![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fsamber%2flo/v1.46.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2fsamber%2flo/v1.46.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2fsamber%2flo/v1.45.0/v1.46.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fsamber%2flo/v1.45.0/v1.46.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>samber/lo (github.com/samber/lo)</summary>

### [`v1.46.0`](https://togithub.com/samber/lo/releases/tag/v1.46.0)

[Compare
Source](https://togithub.com/samber/lo/compare/v1.45.0...v1.46.0)

#### What's Changed

- fix: chunk memory leak, bug fix by
[@&#8203;mihir20](https://togithub.com/mihir20) in
[https://github.com/samber/lo/pull/491](https://togithub.com/samber/lo/pull/491)
- feat: add WaitForWithContext by
[@&#8203;ccoVeille](https://togithub.com/ccoVeille) in
[https://github.com/samber/lo/pull/480](https://togithub.com/samber/lo/pull/480)
- add ForEachCondition implement by
[@&#8203;Sianao](https://togithub.com/Sianao) in
[https://github.com/samber/lo/pull/485](https://togithub.com/samber/lo/pull/485)

#### New Contributors

- [@&#8203;mihir20](https://togithub.com/mihir20) made their first
contribution in
[https://github.com/samber/lo/pull/491](https://togithub.com/samber/lo/pull/491)
- [@&#8203;ccoVeille](https://togithub.com/ccoVeille) made their first
contribution in
[https://github.com/samber/lo/pull/480](https://togithub.com/samber/lo/pull/480)
- [@&#8203;Sianao](https://togithub.com/Sianao) made their first
contribution in
[https://github.com/samber/lo/pull/485](https://togithub.com/samber/lo/pull/485)

**Full Changelog**:
samber/lo@v1.45.0...v1.46.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 11pm every weekday,before 7am
every weekday,every weekend" in timezone Europe/Brussels, Automerge - At
any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/kairos-io/provider-kairos).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40MzEuNCIsInVwZGF0ZWRJblZlciI6IjM3LjQzMS40IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
codeboten referenced this pull request in open-telemetry/opentelemetry-collector-contrib Jul 17, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [github.com/samber/lo](https://togithub.com/samber/lo) | `v1.44.0` ->
`v1.46.0` |
[![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fsamber%2flo/v1.46.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2fsamber%2flo/v1.46.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2fsamber%2flo/v1.44.0/v1.46.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fsamber%2flo/v1.44.0/v1.46.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

<details>
<summary>samber/lo (github.com/samber/lo)</summary>

### [`v1.46.0`](https://togithub.com/samber/lo/releases/tag/v1.46.0)

[Compare
Source](https://togithub.com/samber/lo/compare/v1.45.0...v1.46.0)

#### What's Changed

- fix: chunk memory leak, bug fix by
[@&#8203;mihir20](https://togithub.com/mihir20) in
[https://github.com/samber/lo/pull/491](https://togithub.com/samber/lo/pull/491)
- feat: add WaitForWithContext by
[@&#8203;ccoVeille](https://togithub.com/ccoVeille) in
[https://github.com/samber/lo/pull/480](https://togithub.com/samber/lo/pull/480)
- add ForEachCondition implement by
[@&#8203;Sianao](https://togithub.com/Sianao) in
[https://github.com/samber/lo/pull/485](https://togithub.com/samber/lo/pull/485)

#### New Contributors

- [@&#8203;mihir20](https://togithub.com/mihir20) made their first
contribution in
[https://github.com/samber/lo/pull/491](https://togithub.com/samber/lo/pull/491)
- [@&#8203;ccoVeille](https://togithub.com/ccoVeille) made their first
contribution in
[https://github.com/samber/lo/pull/480](https://togithub.com/samber/lo/pull/480)
- [@&#8203;Sianao](https://togithub.com/Sianao) made their first
contribution in
[https://github.com/samber/lo/pull/485](https://togithub.com/samber/lo/pull/485)

**Full Changelog**:
samber/lo@v1.45.0...v1.46.0

### [`v1.45.0`](https://togithub.com/samber/lo/releases/tag/v1.45.0)

[Compare
Source](https://togithub.com/samber/lo/compare/v1.44.0...v1.45.0)

#### What's Changed

- perf: preallocate in Assign by
[@&#8203;pmalek](https://togithub.com/pmalek) in
[https://github.com/samber/lo/pull/484](https://togithub.com/samber/lo/pull/484)
- feat: adding EarliestBy and LatestBy functions by
[@&#8203;timych](https://togithub.com/timych) in
[https://github.com/samber/lo/pull/489](https://togithub.com/samber/lo/pull/489)

#### New Contributors

- [@&#8203;pmalek](https://togithub.com/pmalek) made their first
contribution in
[https://github.com/samber/lo/pull/484](https://togithub.com/samber/lo/pull/484)
- [@&#8203;timych](https://togithub.com/timych) made their first
contribution in
[https://github.com/samber/lo/pull/489](https://togithub.com/samber/lo/pull/489)

**Full Changelog**:
samber/lo@v1.44.0...v1.45.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "on tuesday" (UTC), Automerge - At any
time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/open-telemetry/opentelemetry-collector-contrib).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40MzEuNCIsInVwZGF0ZWRJblZlciI6IjM3LjQzMS40IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJkZXBlbmRlbmNpZXMiLCJyZW5vdmF0ZWJvdCJdfQ==-->

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: opentelemetrybot <107717825+opentelemetrybot@users.noreply.github.com>
Co-authored-by: Yang Song <songy23@users.noreply.github.com>
@mihir20 mihir20 deleted the mihir/fix-chunk branch July 26, 2024 14:12
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