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

Support rocksdbjava aarch64 build and test #5258

Closed
wants to merge 2 commits into from
Closed

Support rocksdbjava aarch64 build and test #5258

wants to merge 2 commits into from

Conversation

cnqpzhang
Copy link
Contributor

Verified with an Ampere Computing eMAG aarch64 system.

@cnqpzhang
Support rocksdbjava aarch64 build and test
7669b78 
Verified with Ampere Computing eMAG aarch64 system.
@adamretter
Copy link
Collaborator

@cnqpzhang the changes themselves in this PR look good.

@maysamyabandeh I unfortunately have no way of testing this, as I don't have access to an aarch64 platform? Do you have anything available at FB that we could use?

@maysamyabandeh
Copy link
Contributor

@adamretter Unfortunately no. I think as long as the change does not break other platforms and there is a github user confirming that they have tested on the newly added platform, we should be ok with landing the patch.

@cnqpzhang
Copy link
Contributor Author

@maysamyabandeh @adamretter thanks for review. Do you know how the builds for other platforms got generated, for example, 6.0.1 binaries at Maven: http://central.maven.org/maven2/org/rocksdb/rocksdbjni/6.0.1/

@adamretter
Copy link
Collaborator

@cnqpzhang Yup. I build those releases using the rocksdbjavastaticreleasedocker target.

@cnqpzhang
Copy link
Contributor Author

@adamretter Understood (perhaps), it means maven releases could not automatically include aarch64 lib binary unless I would add more things just like what ppc64le did (imagine compiler supported so).

@cnqpzhang
Copy link
Contributor Author

Missed a tiny change #7d54c94 for fixing ARCH to be 64 instead of 32, if $(PLATFORM) is OS_OPENBSD, $(MACHINE) is arch64, I am unable to verify it, which looks very straight-forward and should be fine.

@adamretter
Copy link
Collaborator

@cnqpzhang For releases, IBM/OSUOSL have provided me SSH access to an IBM PowerPC 64LE machine, so I do the ppc64le part of the release build on there.

If someone wants to give me SSH access to other platforms, I am happy to add those to the release process.

@cnqpzhang
Copy link
Contributor Author

cnqpzhang commented May 5, 2019 •
edited
Loading

@adamretter I would take this concern (no aarch64 platform to generate aarch64 binaries for maven release) back to Ampere Computing, and see if anyone could help.

@cnqpzhang
Copy link
Contributor Author

@maysamyabandeh @adamretter Could this pull-request be merged? The two commits are both needed.

@cnqpzhang
Copy link
Contributor Author

cnqpzhang commented May 9, 2019 •
edited
Loading

@maysamyabandeh I unfortunately have no way of testing this, as I don't have access to an aarch64 platform? Do you have anything available at FB that we could use?
@adamretter Unfortunately no.

Hi @maysamyabandeh @adamretter
There would be two options of helping this out,

  1. Try https://github.com/WorksOnArm/cluster/issues, the allocated aarch64 systems can serve for temp use, but it may not guarantee a long-term env for release builds.
  2. I raised this issue in team. Ampere (at Santa Clara) could send a dev system to FB for this, and other similar dev/testing work (if any) on aarch64 . If okay @maysamyabandeh please write to my email (I cannot find yours) and I’d like to set up the connection for detailed follow-ups.

@siying
Copy link
Contributor

siying commented May 9, 2019

@adamretter is there qualified hosts in Cloud providers like AWS EC2? We can discuss whether it is possible reimburse the cost for the use.

@adamretter
Copy link
Collaborator

@siying Yes both AWS EC2 and Scaleway are the aarch64 providers that I know, there may also be others.

However... just to be clear, I am not in any way saying that I can support the aarch64 platform! I am saying that I could perform release builds on aarch64 and report if the builds fail ;-)

@cnqpzhang
Copy link
Contributor Author

cnqpzhang commented May 10, 2019 •
edited
Loading

@siying @adamretter Packet (similar as Scaleway) can be an option as well, if FB does not want to maintain a physical aarch64 system in office. In addition, I may have a more convenient channel to help access systems there, if selected Ampere eMAG.

About the overall aarch64 support, I haven't seen much work by far, almost all linux64 source and scripts can be used directly. Maybe my estimation is rough, while the 1st step should be "not to break other platforms", which can be easily achieved and verified. Enabling aarch64 build with this PR helped me successfully run Flink and flink-benchmarks, FYI.

@cnqpzhang
Copy link
Contributor Author

Hi @adamretter, I wonder whether this pull-in request can be merged suppose no further concerns about the code changes and on the condition that it would not bring any negative impact on other builds. Thanks.

facebook-github-bot
facebook-github-bot reviewed Jun 13, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@maysamyabandeh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@maysamyabandeh merged this pull request in 5c76ba9.

vagogte pushed a commit to vagogte/rocksdb that referenced this pull request Jun 18, 2019
@cnqpzhang @facebook-github-bot
Support rocksdbjava aarch64 build and test (facebook#5258)
57c5c82 
Summary:
Verified with an Ampere Computing eMAG aarch64 system.
Pull Request resolved: facebook#5258

Differential Revision: D15807309

Pulled By: maysamyabandeh

fbshipit-source-id: ab85d2fd3fe40e6094430ab0eba557b1e979510d
@Yikun Yikun mentioned this pull request Aug 6, 2019
Closed
@wangxiyuan
Copy link

@cnqpzhang, When I run test on ARM, I hit these tests failures: make check_some #5736 and make jtest #5751

Do you hit them as well? Do you have any suggestion?

Thanks very much.

@jiameixie jiameixie mentioned this pull request Dec 16, 2019
Closed
adamretter pushed a commit that referenced this pull request Dec 20, 2019
@cnqpzhang @adamretter
Support rocksdbjava aarch64 build and test (#5258)
c75cb1e 
Summary:
Verified with an Ampere Computing eMAG aarch64 system.
Pull Request resolved: #5258

Differential Revision: D15807309

Pulled By: maysamyabandeh

fbshipit-source-id: ab85d2fd3fe40e6094430ab0eba557b1e979510d
siying pushed a commit that referenced this pull request Jan 6, 2020
@adamretter @guyuqi
@cnqpzhang @Yikun @siying
ARM64 commits to 5.18.3 to create 5.18.4 (#6250)
689c155 
* RocksDB CRC32c optimization with ARMv8 Intrinsic (#5221)

Summary:
1. Add Arm linear crc32c implemtation for RocksDB.
2. Arm runtime check for crc32
Pull Request resolved: #5221

Differential Revision: D15013685

Pulled By: siying

fbshipit-source-id: 2c2983743d26656d93f212dc7c1a3cf66a1acf12

* Support rocksdbjava aarch64 build and test (#5258)

Summary:
Verified with an Ampere Computing eMAG aarch64 system.
Pull Request resolved: #5258

Differential Revision: D15807309

Pulled By: maysamyabandeh

fbshipit-source-id: ab85d2fd3fe40e6094430ab0eba557b1e979510d

* Cleanup the Arm64 CRC32 unused warning (#5565)

Summary:
When 'HAVE_ARM64_CRC' is set, the blew methods:

- bool rocksdb::crc32c::isSSE42()
- bool rocksdb::crc32c::isPCLMULQDQ()

are defined but not used, the unused-function is raised
when do rocksdb build.

This patch try to cleanup these warnings by add ifndef,
if it build under the HAVE_ARM64_CRC, we will not define
`isSSE42` and `isPCLMULQDQ`.
Pull Request resolved: #5565

Differential Revision: D16233654

fbshipit-source-id: c32a9dda7465dbf65f9ccafef159124db92cdffd

* Fixes for building RocksJava releases on arm64v8

Summary: Pull Request resolved: #5674

Differential Revision: D16870338

fbshipit-source-id: c8dac644b1479fa734b491f3a8d50151772290f7

* Remove invalid comparison of va_list and nullptr (#5836)

Summary:
The comparison of va_list and nullptr is always False under any arch, and will raise invalid operands of types error in aarch64 env (`error: invalid operands of types ‘va_list {aka __va_list}’ and ‘std::nullptr_t’ to binary ‘operator!=’`).

This patch removes this invalid assert.

Closes: #4277
Pull Request resolved: #5836

Differential Revision: D17532470

fbshipit-source-id: ca98078ecbc6a9416c69de3bd6ffcfa33a0f0185

* Fix naming of library on PPC64LE (#6080)

Summary:
**NOTE**: This also needs to be back-ported to be 6.4.6

Fix a regression introduced in f2bf0b2 by #5674 whereby the compiled library would get the wrong name on PPC64LE platforms.

On PPC64LE, the regression caused the library to be named `librocksdbjni-linux64.so` instead of `librocksdbjni-linux-ppc64le.so`.

This PR corrects the name back to `librocksdbjni-linux-ppc64le.so` and also corrects the ordering of conditional arguments in the Makefile to match the expected order as defined in the documentation for Make.
Pull Request resolved: #6080

Differential Revision: D18710351

fbshipit-source-id: d4db87ef378263b57de7f9edce1b7d15644cf9de

Co-authored-by: Yuqi <yuqi.gu@arm.com>
Co-authored-by: Patrick Zhang <cnqpzhang@163.com>
Co-authored-by: Yikun Jiang <yikunkero@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@adamretter adamretter Awaiting requested review from adamretter

Assignees

@adamretter adamretter

Labels
CLA Signed java-api Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@cnqpzhang @adamretter @maysamyabandeh @siying @facebook-github-bot @wangxiyuan