ARROW-4905: [C++][Plasma] Remove dlmalloc symbols from client library #3927
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 tasks
Member
|
This needs to be rebased after merging #3928. It would probably be a good idea to check that the static/shared-only builds aren't broken by this |
wesm
force-pushed
the
split-plasma-server-client
branch
from
e24f56f to
7fd7bd0
Compare
Member
|
Rebased and fixed merge conflict |
pitrou
reviewed
Mar 18, 2019
Member
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
Looks worthwhile in principle, just two comments.
cpp/src/plasma/malloc.h
Outdated
| int64_t GetMmapSize(int fd); | ||
|
|
||
| void SetMallocGranularity(int value); | ||
| struct mmap_record { |
Member
There was a problem hiding this comment.
Can't you put all this in a namespace?
Contributor
Author
There was a problem hiding this comment.
Yes, I put everything in a namespace now!
cpp/src/plasma/dlmalloc.cc
Outdated
| return -1; | ||
| } | ||
|
|
||
| FILE* file = fdopen(fd, "a+"); |
Member
There was a problem hiding this comment.
I realize this is just being moved around, but FTR I fail to understand what the file achieves. It's not used in any case below.
Contributor
Author
There was a problem hiding this comment.
Yeah, I removed it (both fdopen and also the dup further below).
cmake changes add missing file fixes cmake-format revert dlmalloc.c rename clang-format fix shuffle more files update
pcmoritz
force-pushed
the
split-plasma-server-client
branch
from
7fd7bd0 to
ab5f616
Compare
Codecov Report
@@ Coverage Diff @@
## master #3927 +/- ##
==========================================
+ Coverage 87.83% 88.64% +0.81%
==========================================
Files 728 595 -133
Lines 89822 80188 -9634
Branches 1252 0 -1252
==========================================
- Hits 78891 71086 -7805
+ Misses 10814 9102 -1712
+ Partials 117 0 -117
Continue to review full report at Codecov.
|
Contributor
Author
|
@wesm: The static linking docker compose test if failing with for me. I'm pretty sure it's unrelated to this PR, any idea what is going on? |
Member
|
Not sure, it might be a link order thing. Can you please open a JIRA? |
Contributor
Author
Member
|
See #3974 for a fix |
pcmoritz
added a commit
to pcmoritz/arrow
that referenced
this pull request
Mar 19, 2019
This separates the allocator into a part that is only linked into the store (dlmalloc.cc, containing dlmalloc) and a part that is only linked into the client (malloc.cc, not containing dlmalloc). Also a small build system cleanup (not adding the thirdparty/ directory to the include path). Author: Philipp Moritz <pcmoritz@gmail.com> Closes apache#3927 from pcmoritz/split-plasma-server-client and squashes the following commits: 77cd7cd <Philipp Moritz> update d5de8fc <Philipp Moritz> clean up plasma file descriptor handling 1c979d2 <Philipp Moritz> update 57903be <Philipp Moritz> update aa40a2b <Philipp Moritz> put functions into namespace 81e53ac <Philipp Moritz> increase timeout ab5f616 <Philipp Moritz> separate client and store code
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This separates the allocator into a part that is only linked into the store (dlmalloc.cc, containing dlmalloc) and a part that is only linked into the client (malloc.cc, not containing dlmalloc). Also a small build system cleanup (not adding the thirdparty/ directory to the include path).