Skip to content

Conversation

@gf2121
Copy link
Contributor

@gf2121 gf2121 commented Dec 11, 2022

@github-actions
Copy link

@github-actions
Copy link

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

@github-actions
Copy link

⚠️ GitHub issue #14912 has no components, please add labels for components.

@gf2121 gf2121 marked this pull request as draft December 11, 2022 14:57
Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

Since we also maintain a copy of sun.misc.Unsafe in MemoryUtil, can we add our own getInt, getLong, etc. there and replace Netty Unsafe with our Unsafe? That way we can at least keep the same behavior as before.

@gf2121
Copy link
Contributor Author

gf2121 commented Dec 12, 2022

Thanks @lidavidm for feedback!

I think you are meaning to delegate methods on Unsafe in MemoryUtil (like what PlatformDependent do, just discard the params around netty). But i'm also seeing that ArrowBuf directly replaced PlatformDependent.xxx with MemoryUtil.UNSAFE.xxx in #6131. So I wonder if the logic in PlatformDependent(like this) is still wanted:

  • If yes, we can introduce delegating methods in this PR and replaces the direct operation on MemoryUtil#UNSAFE in a follow up.

  • If no, we can just operate on MemoryUtil#UNSAFE without introducing extra methods in MemoryUtil.

@lidavidm
Copy link
Member

Using MemoryUtil.UNSAFE directly is OK, IMO.

@gf2121 gf2121 force-pushed the GH-14912 branch 2 times, most recently from b5022b7 to 17a1c35 Compare December 13, 2022 13:26
@gf2121 gf2121 marked this pull request as ready for review December 13, 2022 13:36
@gf2121 gf2121 changed the title GH-14912: [Java] Remove usage of PlatformDependent in arrow-vector GH-14912: [Java] Remove usage of PlatformDependent in arrow-vector, arrow-jdbc and arrow-algorithm Dec 13, 2022
@gf2121
Copy link
Contributor Author

gf2121 commented Dec 13, 2022

PlatformDependent was used in arrow-algorithm, arrow-jdbc, arrow-memory-netty, arrow-vector. I've removed the usages in arrow-algorithm, arrow-jdbc and arrow-vector, and removed the dependency of netty-common in arrow-algorithm and arrow-jdbc. It would take a bit more job to remove dependency of netty-common in arrow-vector as it is using some Netty collection utils llike IntObjectMap. Maybe do that in follow up.

Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

Thanks!

CC @lwhite1 you may want to glance through this too

@lidavidm
Copy link
Member

I filed #14936 for dropping Netty entirely.

@maosuhan
Copy link

maosuhan commented Feb 1, 2023

How about merging this MR first? We are also blocked by this netty unsafe issue. @lidavidm

@lidavidm
Copy link
Member

lidavidm commented Feb 1, 2023

I rebased and will merge after that

@lidavidm lidavidm merged commit 5618a47 into apache:master Feb 1, 2023
@ursabot
Copy link

ursabot commented Feb 1, 2023

Benchmark runs are scheduled for baseline = 5765aa2 and contender = 5618a47. 5618a47 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed ⬇️0.0% ⬆️0.0% ⚠️ Contender and baseline run contexts do not match] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.45% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.0% ⬆️0.0% ⚠️ Contender and baseline run contexts do not match] ursa-i9-9960x
[Finished ⬇️0.0% ⬆️0.0% ⚠️ Contender and baseline run contexts do not match] ursa-thinkcentre-m75q
Buildkite builds:
[Failed] 5618a477 ec2-t3-xlarge-us-east-2
[Failed] 5618a477 test-mac-arm
[Failed] 5618a477 ursa-i9-9960x
[Finished] 5618a477 ursa-thinkcentre-m75q
[Finished] 5765aa29 ec2-t3-xlarge-us-east-2
[Failed] 5765aa29 test-mac-arm
[Failed] 5765aa29 ursa-i9-9960x
[Finished] 5765aa29 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

pribor pushed a commit to GlobalWebIndex/arrow that referenced this pull request Oct 24, 2025
…tor, arrow-jdbc and arrow-algorithm (apache#14913)

* Closes: apache#14912

Authored-by: 郭峰 <guofeng.my@bytedance.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Java] Remove usage of PlatformDependent in arrow-vector

4 participants