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

fix: chunk memory leak, bug fix #491

Merged
merged 1 commit into from
Jul 15, 2024
Merged

fix: chunk memory leak, bug fix #491

merged 1 commit into from
Jul 15, 2024

Conversation

mihir20
Copy link
Contributor

@mihir20 mihir20 commented Jul 15, 2024

raising a fix as mentioned in: #438

ccoVeille
ccoVeille approved these changes Jul 15, 2024
View reviewed changes
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 👍

slice.go
@@ -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.

ref: https://stackoverflow.com/questions/12768744/re-slicing-slices-in-golang/18911267#18911267

slice_test.go
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
Open
@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
@renovate
fix(deps): update module github.com/samber/lo to v1.46.0 (#604)
8bdabec 
[![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
@renovate @opentelemetrybot @songy23
Update module github.com/samber/lo to v1.46.0 (#34113)
892692a 
[![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
Reviewers

@ccoVeille ccoVeille ccoVeille approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mihir20 @samber @ccoVeille