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

ARROW-16981: [C++] Expose jemalloc statistics for logging #13516

Merged
merged 13 commits into from
Sep 21, 2022
Merged

ARROW-16981: [C++] Expose jemalloc statistics for logging #13516

merged 13 commits into from
Sep 21, 2022

Conversation

rok
Copy link
Member

@rok rok commented Jul 5, 2022

This is to resolve ARROW-16981.

@github-actions
Copy link

github-actions bot commented Jul 5, 2022

https://issues.apache.org/jira/browse/ARROW-16981

@github-actions
Copy link

github-actions bot commented Jul 5, 2022

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

lidavidm
lidavidm reviewed Jul 5, 2022
View reviewed changes
cpp/src/arrow/memory_pool_jemalloc.cc Outdated Show resolved Hide resolved
cpp/src/arrow/memory_pool.h Outdated Show resolved Hide resolved
pitrou
pitrou reviewed Jul 11, 2022
View reviewed changes
cpp/src/arrow/memory_pool.h Outdated Show resolved Hide resolved
cpp/src/arrow/memory_pool.h Outdated Show resolved Hide resolved
cpp/src/arrow/memory_pool.h Outdated Show resolved Hide resolved
cpp/src/arrow/memory_pool_jemalloc.cc Outdated Show resolved Hide resolved
@rok rok force-pushed the ARROW-16981 branch 2 times, most recently from b58b1d9 to 3ccabc3 Compare September 5, 2022 19:51
lidavidm
lidavidm reviewed Sep 6, 2022
View reviewed changes
cpp/src/arrow/memory_pool.cc Outdated Show resolved Hide resolved
cpp/src/arrow/memory_pool.h Outdated Show resolved Hide resolved
@rok rok marked this pull request as ready for review September 8, 2022 20:04
@rok rok force-pushed the ARROW-16981 branch 2 times, most recently from 9dac9b4 to 307063d Compare September 8, 2022 20:29
@rok
Copy link
Member Author

rok commented Sep 9, 2022

@pitrou any idea why is overloading not allowed? It builds and works ok locally.

@lidavidm
Copy link
Member

lidavidm commented Sep 9, 2022

presumably because uint64_t and size_t are actually the same type on those platforms (also, we use raw pointers for out parameters, not references, typically)

pitrou
pitrou reviewed Sep 9, 2022
View reviewed changes
cpp/src/arrow/memory_pool.h Outdated Show resolved Hide resolved
pitrou
pitrou reviewed Sep 9, 2022
View reviewed changes
cpp/src/arrow/memory_pool.h Outdated Show resolved Hide resolved
pitrou
pitrou reviewed Sep 9, 2022
View reviewed changes
cpp/src/arrow/memory_pool.h Outdated Show resolved Hide resolved
@rok rok force-pushed the ARROW-16981 branch 3 times, most recently from 2224027 to d1b68ed Compare September 9, 2022 21:20
@rok
Copy link
Member Author

rok commented Sep 9, 2022

Do we want Python binding for these as well?

@pitrou
Copy link
Member

pitrou commented Sep 10, 2022

I don't know, why not? Can be a separate JIRA, though.

@rok
Copy link
Member Author

rok commented Sep 12, 2022

Added ARROW-17685 for the Python wrapper.

@rok rok force-pushed the ARROW-16981 branch 6 times, most recently from 1df8c26 to f06e125 Compare September 15, 2022 13:51
rok
rok commented Sep 17, 2022
View reviewed changes
cpp/src/arrow/flight/sql/types.h Outdated Show resolved Hide resolved
benibus
benibus reviewed Sep 20, 2022
View reviewed changes
Copy link
Collaborator

@benibus benibus left a comment

Choose a reason for hiding this comment

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

Here are some potential changes for implementing the callback API with std::function, which uses closures instead of a client-provided void*. I updated the relevant tests as well.

Hopefully this is the correct method of suggesting patches... the github diffs appear to be against the base branch, not the latest commit.

cpp/src/arrow/memory_pool.h Outdated Show resolved Hide resolved
cpp/src/arrow/memory_pool_jemalloc.cc Outdated Show resolved Hide resolved
cpp/src/arrow/memory_pool.cc Outdated Show resolved Hide resolved
cpp/src/arrow/memory_pool_test.cc Outdated Show resolved Hide resolved
@rok
Copy link
Member Author

rok commented Sep 20, 2022 •
edited
Loading

Here are some potential changes for implementing the callback API with std::function, which uses closures instead of a client-provided void*. I updated the relevant tests as well.

This looks good to me! Thanks a lot @benibus !

Hopefully this is the correct method of suggesting patches... the github diffs appear to be against the base branch, not the latest commit.

Well this was pretty nice for me but maybe extra work for you :). I added you as a collaborator to my fork in case you'd want to push more changes to this branch and avoid the UI.

I've also linted and rebased.

@rok @benibus
Apply suggestions from code review
1af3ef5 
Co-authored-by: Ben Harkins <60872452+benibus@users.noreply.github.com>
@rok rok requested a review from pitrou September 20, 2022 10:22
pitrou
pitrou requested changes Sep 21, 2022
View reviewed changes
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Thanks for the update. Some further comments below, but looks good on the principle.

cpp/src/arrow/memory_pool_jemalloc.cc Outdated Show resolved Hide resolved
cpp/src/arrow/memory_pool_jemalloc.cc Outdated
auto cb_wrapper = [](void* opaque, const char* str) {
(*static_cast<std::function<void(const char*)>*>(opaque))(str);
};
if (write_cb) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this required? I think the caller should make sure that write_cb is initialized.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think not but not sure. I removed it for now. @benibus do you think this would be needed?

cpp/src/arrow/memory_pool_test.cc Outdated Show resolved Hide resolved
cpp/src/arrow/memory_pool_test.cc Outdated Show resolved Hide resolved
rok and others added 2 commits September 21, 2022 13:01
@rok @pitrou
Update cpp/src/arrow/memory_pool_test.cc
5c2eefc 
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
@rok
Review feedback
8f3b32f
@rok
Copy link
Member Author

rok commented Sep 21, 2022 •
edited
Loading

Thanks for the review @pitrou! I've addressed your points.

@pitrou pitrou merged commit 8d11e3d into apache:master Sep 21, 2022
@pitrou
Copy link
Member

pitrou commented Sep 21, 2022

Thanks a lot @rok and @benibus !

@rok
Copy link
Member Author

rok commented Sep 21, 2022

Thank you @pitrou & @benibus! :)

@ursabot
Copy link

ursabot commented Sep 21, 2022

Benchmark runs are scheduled for baseline = bd433c0 and contender = 8d11e3d. 8d11e3d 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 ⬇️0.14% ⬆️0.1%] test-mac-arm
[Failed ⬇️0.28% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.78% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 8d11e3dd ec2-t3-xlarge-us-east-2
[Failed] 8d11e3dd test-mac-arm
[Failed] 8d11e3dd ursa-i9-9960x
[Finished] 8d11e3dd ursa-thinkcentre-m75q
[Finished] bd433c01 ec2-t3-xlarge-us-east-2
[Finished] bd433c01 test-mac-arm
[Failed] bd433c01 ursa-i9-9960x
[Finished] bd433c01 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

zagto pushed a commit to zagto/arrow that referenced this pull request Oct 7, 2022
@rok @zagto
ARROW-16981: [C++] Expose jemalloc statistics for logging (apache#13516)
93f37b7 
This is to resolve [ARROW-16981](https://issues.apache.org/jira/browse/ARROW-16981).

Lead-authored-by: Rok <rok@mihevc.org>
Co-authored-by: Rok Mihevc <rok@mihevc.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
fatemehp pushed a commit to fatemehp/arrow that referenced this pull request Oct 17, 2022
@rok @fatemehp
ARROW-16981: [C++] Expose jemalloc statistics for logging (apache#13516)
c07b789 
This is to resolve [ARROW-16981](https://issues.apache.org/jira/browse/ARROW-16981).

Lead-authored-by: Rok <rok@mihevc.org>
Co-authored-by: Rok Mihevc <rok@mihevc.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@asfimport asfimport mentioned this pull request Sep 21, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lidavidm lidavidm lidavidm left review comments

@benibus benibus benibus left review comments

@pitrou pitrou pitrou approved these changes

Assignees
No one assigned
Labels
Component: C++ Component: FlightRPC
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@rok @lidavidm @pitrou @ursabot @benibus