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

GH-35212: [Go] Add ability to show full call stack with ARROW_CHECKED_MAX_RETAINED_FRAMES #35215

Merged
merged 4 commits into from
Apr 18, 2023

Conversation

hermanschaaf
Copy link
Contributor

@hermanschaaf hermanschaaf commented Apr 18, 2023

This adds support for printing the full call stack when a leak is reported by the checked memory allocator. An ARROW_CHECKED_MAX_RETAINED_FRAMES environment variable controls how many frames are retained. To keep this completely backwards-compatible, the default right now is zero. In this case, the reported error is exactly the same as before. When a higher value is given though, a longer call stack is printed. For example:

Before (same as ARROW_CHECKED_MAX_RETAINED_FRAMES=0):

checked_allocator.go:160: LEAK of 64 bytes FROM github.com/apache/arrow/go/v12/arrow/array.(*TimestampBuilder).newData line 2396

After (with ARROW_CHECKED_MAX_RETAINED_FRAMES=100):

checked_allocator.go:160: LEAK of 64 bytes FROM github.com/apache/arrow/go/v12/arrow/array.(*TimestampBuilder).newData line 2396
     github.com/apache/arrow/go/v12/arrow/array.(*TimestampBuilder).NewTimestampArray line 2386
     github.com/apache/arrow/go/v12/arrow/array.(*TimestampBuilder).NewArray line 2380
     github.com/apache/arrow/go/v12/arrow/array.(*RecordBuilder).NewRecord line 346
     github.com/cloudquery/plugin-sdk/v2/testdata.GenTestData line 243
     github.com/cloudquery/plugin-sdk/v2/plugins/destination.testMigration line 53
     github.com/cloudquery/plugin-sdk/v2/plugins/destination.(*PluginTestSuite).destinationPluginTestMigrate.func5 line 239
     testing.tRunner line 1576
     runtime.goexit line 1172

@hermanschaaf hermanschaaf changed the title Add ability to show full call stack with ARROW_CHECKED_MAX_RETAINED_FRAMES GH-35212: [Go] Add ability to show full call stack with ARROW_CHECKED_MAX_RETAINED_FRAMES Apr 18, 2023
@github-actions github-actions bot added the awaiting review Awaiting review label Apr 18, 2023
@github-actions
Copy link

@github-actions
Copy link

⚠️ GitHub issue #35212 has been automatically assigned in GitHub to PR creator.

Copy link
Member

@zeroshade zeroshade left a comment

Choose a reason for hiding this comment

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

this looks awesome, thanks for this!

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Apr 18, 2023
@zeroshade zeroshade merged commit 7a7b414 into apache:main Apr 18, 2023
@ursabot
Copy link

ursabot commented Apr 20, 2023

Benchmark runs are scheduled for baseline = 16bd95f and contender = 7a7b414. 7a7b414 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Finished ⬇️9.18% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.42% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 7a7b414d ec2-t3-xlarge-us-east-2
[Failed] 7a7b414d test-mac-arm
[Finished] 7a7b414d ursa-i9-9960x
[Finished] 7a7b414d ursa-thinkcentre-m75q
[Finished] 16bd95fd ec2-t3-xlarge-us-east-2
[Failed] 16bd95fd test-mac-arm
[Finished] 16bd95fd ursa-i9-9960x
[Finished] 16bd95fd ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@ursabot
Copy link

ursabot commented Apr 20, 2023

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

zeroshade pushed a commit that referenced this pull request Apr 21, 2023
Follow-up to #35215. It's mostly cosmetic, but without the additional 2 skips passed in to `Callers`, the Caller frame is repeated in the stack trace, and one frame below it is added as well. With this change, the checked allocator stack trace contains no duplicates.

I had this change locally but didn't realize it wasn't pushed to the PR branch, sorry about that 🙇
* Closes: #35234

Authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
liujiacheng777 pushed a commit to LoongArch-Python/arrow that referenced this pull request May 11, 2023
…HECKED_MAX_RETAINED_FRAMES (apache#35215)

This adds support for printing the full call stack when a leak is reported by the checked memory allocator. An `ARROW_CHECKED_MAX_RETAINED_FRAMES` environment variable controls how many frames are retained. To keep this completely backwards-compatible, the default right now is zero. In this case, the reported error is exactly the same as before. When a higher value is given though, a longer call stack is printed. For example:

Before (same as `ARROW_CHECKED_MAX_RETAINED_FRAMES=0`):

```
checked_allocator.go:160: LEAK of 64 bytes FROM github.com/apache/arrow/go/v12/arrow/array.(*TimestampBuilder).newData line 2396
```

After (with `ARROW_CHECKED_MAX_RETAINED_FRAMES=100`):

```
checked_allocator.go:160: LEAK of 64 bytes FROM github.com/apache/arrow/go/v12/arrow/array.(*TimestampBuilder).newData line 2396
     github.com/apache/arrow/go/v12/arrow/array.(*TimestampBuilder).NewTimestampArray line 2386
     github.com/apache/arrow/go/v12/arrow/array.(*TimestampBuilder).NewArray line 2380
     github.com/apache/arrow/go/v12/arrow/array.(*RecordBuilder).NewRecord line 346
     github.com/cloudquery/plugin-sdk/v2/testdata.GenTestData line 243
     github.com/cloudquery/plugin-sdk/v2/plugins/destination.testMigration line 53
     github.com/cloudquery/plugin-sdk/v2/plugins/destination.(*PluginTestSuite).destinationPluginTestMigrate.func5 line 239
     testing.tRunner line 1576
     runtime.goexit line 1172
```
* Closes: apache#35212

Authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
liujiacheng777 pushed a commit to LoongArch-Python/arrow that referenced this pull request May 11, 2023
Follow-up to apache#35215. It's mostly cosmetic, but without the additional 2 skips passed in to `Callers`, the Caller frame is repeated in the stack trace, and one frame below it is added as well. With this change, the checked allocator stack trace contains no duplicates.

I had this change locally but didn't realize it wasn't pushed to the PR branch, sorry about that 🙇
* Closes: apache#35234

Authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
ArgusLi pushed a commit to Bit-Quill/arrow that referenced this pull request May 15, 2023
…HECKED_MAX_RETAINED_FRAMES (apache#35215)

This adds support for printing the full call stack when a leak is reported by the checked memory allocator. An `ARROW_CHECKED_MAX_RETAINED_FRAMES` environment variable controls how many frames are retained. To keep this completely backwards-compatible, the default right now is zero. In this case, the reported error is exactly the same as before. When a higher value is given though, a longer call stack is printed. For example:

Before (same as `ARROW_CHECKED_MAX_RETAINED_FRAMES=0`):

```
checked_allocator.go:160: LEAK of 64 bytes FROM github.com/apache/arrow/go/v12/arrow/array.(*TimestampBuilder).newData line 2396
```

After (with `ARROW_CHECKED_MAX_RETAINED_FRAMES=100`):

```
checked_allocator.go:160: LEAK of 64 bytes FROM github.com/apache/arrow/go/v12/arrow/array.(*TimestampBuilder).newData line 2396
     github.com/apache/arrow/go/v12/arrow/array.(*TimestampBuilder).NewTimestampArray line 2386
     github.com/apache/arrow/go/v12/arrow/array.(*TimestampBuilder).NewArray line 2380
     github.com/apache/arrow/go/v12/arrow/array.(*RecordBuilder).NewRecord line 346
     github.com/cloudquery/plugin-sdk/v2/testdata.GenTestData line 243
     github.com/cloudquery/plugin-sdk/v2/plugins/destination.testMigration line 53
     github.com/cloudquery/plugin-sdk/v2/plugins/destination.(*PluginTestSuite).destinationPluginTestMigrate.func5 line 239
     testing.tRunner line 1576
     runtime.goexit line 1172
```
* Closes: apache#35212

Authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
ArgusLi pushed a commit to Bit-Quill/arrow that referenced this pull request May 15, 2023
Follow-up to apache#35215. It's mostly cosmetic, but without the additional 2 skips passed in to `Callers`, the Caller frame is repeated in the stack trace, and one frame below it is added as well. With this change, the checked allocator stack trace contains no duplicates.

I had this change locally but didn't realize it wasn't pushed to the PR branch, sorry about that 🙇
* Closes: apache#35234

Authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
rtpsw pushed a commit to rtpsw/arrow that referenced this pull request May 16, 2023
…HECKED_MAX_RETAINED_FRAMES (apache#35215)

This adds support for printing the full call stack when a leak is reported by the checked memory allocator. An `ARROW_CHECKED_MAX_RETAINED_FRAMES` environment variable controls how many frames are retained. To keep this completely backwards-compatible, the default right now is zero. In this case, the reported error is exactly the same as before. When a higher value is given though, a longer call stack is printed. For example:

Before (same as `ARROW_CHECKED_MAX_RETAINED_FRAMES=0`):

```
checked_allocator.go:160: LEAK of 64 bytes FROM github.com/apache/arrow/go/v12/arrow/array.(*TimestampBuilder).newData line 2396
```

After (with `ARROW_CHECKED_MAX_RETAINED_FRAMES=100`):

```
checked_allocator.go:160: LEAK of 64 bytes FROM github.com/apache/arrow/go/v12/arrow/array.(*TimestampBuilder).newData line 2396
     github.com/apache/arrow/go/v12/arrow/array.(*TimestampBuilder).NewTimestampArray line 2386
     github.com/apache/arrow/go/v12/arrow/array.(*TimestampBuilder).NewArray line 2380
     github.com/apache/arrow/go/v12/arrow/array.(*RecordBuilder).NewRecord line 346
     github.com/cloudquery/plugin-sdk/v2/testdata.GenTestData line 243
     github.com/cloudquery/plugin-sdk/v2/plugins/destination.testMigration line 53
     github.com/cloudquery/plugin-sdk/v2/plugins/destination.(*PluginTestSuite).destinationPluginTestMigrate.func5 line 239
     testing.tRunner line 1576
     runtime.goexit line 1172
```
* Closes: apache#35212

Authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
rtpsw pushed a commit to rtpsw/arrow that referenced this pull request May 16, 2023
Follow-up to apache#35215. It's mostly cosmetic, but without the additional 2 skips passed in to `Callers`, the Caller frame is repeated in the stack trace, and one frame below it is added as well. With this change, the checked allocator stack trace contains no duplicates.

I had this change locally but didn't realize it wasn't pushed to the PR branch, sorry about that 🙇
* Closes: apache#35234

Authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
@candiduslynx candiduslynx deleted the add-checked-allocator-frames branch June 2, 2023 11:57
kou pushed a commit to apache/arrow-go that referenced this pull request Aug 30, 2024
Follow-up to apache/arrow#35215. It's mostly cosmetic, but without the additional 2 skips passed in to `Callers`, the Caller frame is repeated in the stack trace, and one frame below it is added as well. With this change, the checked allocator stack trace contains no duplicates.

I had this change locally but didn't realize it wasn't pushed to the PR branch, sorry about that 🙇
* Closes: #35234

Authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting merge Awaiting merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Go] Add more frames to Checked Allocator memory leak output
3 participants