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

ARM64 commits to 5.18.3 to create 5.18.4 #6250

Merged
merged 6 commits into from
Jan 6, 2020

Conversation

adamretter
Copy link
Collaborator

@adamretter adamretter commented Dec 27, 2019

In response to #6188

As requested by @siying

guyuqi and others added 6 commits December 20, 2019 12:14
Summary:
1. Add Arm linear crc32c implemtation for RocksDB.
2. Arm runtime check for crc32
Pull Request resolved: facebook#5221

Differential Revision: D15013685

Pulled By: siying

fbshipit-source-id: 2c2983743d26656d93f212dc7c1a3cf66a1acf12
Summary:
Verified with an Ampere Computing eMAG aarch64 system.
Pull Request resolved: facebook#5258

Differential Revision: D15807309

Pulled By: maysamyabandeh

fbshipit-source-id: ab85d2fd3fe40e6094430ab0eba557b1e979510d
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: facebook#5565

Differential Revision: D16233654

fbshipit-source-id: c32a9dda7465dbf65f9ccafef159124db92cdffd
Summary: Pull Request resolved: facebook#5674

Differential Revision: D16870338

fbshipit-source-id: c8dac644b1479fa734b491f3a8d50151772290f7
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: facebook#4277
Pull Request resolved: facebook#5836

Differential Revision: D17532470

fbshipit-source-id: ca98078ecbc6a9416c69de3bd6ffcfa33a0f0185
Summary:
**NOTE**: This also needs to be back-ported to be 6.4.6

Fix a regression introduced in f2bf0b2 by facebook#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: facebook#6080

Differential Revision: D18710351

fbshipit-source-id: d4db87ef378263b57de7f9edce1b7d15644cf9de
@adamretter adamretter added the arm label Dec 27, 2019
@adamretter adamretter requested a review from siying December 27, 2019 13:00
@siying siying merged commit 689c155 into facebook:5.18.fb Jan 6, 2020
@jiameixie
Copy link

@adamretter
Thanks for your efforts to release 5.18.4.
How can we get RocksDB JNI-5.18.4 from Maven central? Thx!

@adamretter
Copy link
Collaborator Author

@jiameixe I think there is an issue still on Arm that I made @siying aware of. I was waiting to hear from him on that. After that, once FB tag the release I can get it up onto Maven Central

@siying
Copy link
Contributor

siying commented Jan 9, 2020

@adamretter I can confirm that it is a bug somewhere, but not related to ARM. It's just somehow got triggered because the ARM environment. But it's bug is in core RocksDB, which we need to spend time to figure out. But the release should be good to go.

@adamretter
Copy link
Collaborator Author

@siying okie dokie. Can you create the v5.18.4 tag please? I don't have permission :-(

@jiameixie
Copy link

@adamretter @siying Have v5.18.4 been released? I couldn't find it.

@guyuqi
Copy link
Contributor

guyuqi commented Feb 11, 2020

Hi @siying, could you please help to create the v5.18.4 tag? Thank you very much!

@siying
Copy link
Contributor

siying commented Feb 11, 2020

One thing I am worried is that if I tagged it, will it become "Latest Release" in github?

@guyuqi
Copy link
Contributor

guyuqi commented Feb 11, 2020

Hi @siying, could you kindly try to do :

  1. click into the tag ->
  2. click the 'Edit tag' button ->
  3. add a value for the 'Release title' field.
    It may convert the tag to a release and moved the 'Latest Release' badge to the correct release.
    Thanks a lot ! :-)

@siying
Copy link
Contributor

siying commented Feb 11, 2020

OK. Now I tagged it.

@adamretter
Copy link
Collaborator Author

@siying I am not sure what happened but the tag looks wrong :-/

If you compare:

  1. the branch https://github.com/facebook/rocksdb/commits/5.18.fb
  2. the tag https://github.com/facebook/rocksdb/commits/v5.18.4

You will see that the branch has the correct ARM64 commits at its head, but the tag does not. Instead the tag has an additional commit which hasn't come from the branch

Also the file include/rocksdb/version.h in the 5.18.fb branch seems to have not been updated.
Whilst the same file in the v5.18.4 tag is very out of date (5.8.8), and seems to imply that that tag was taken from the wrong base.

@siying could you take a look please?

@adamretter
Copy link
Collaborator Author

@siying could you take a look please?

@pdillinger
Copy link
Contributor

@adamretter Should be fixed now. https://github.com/facebook/rocksdb/releases/tag/v5.18.4 Read the note about updated tags.

@jiameixie
Copy link

@pdillinger @adamretter @siying Has it been published to maven central? I can't find this in https://mvnrepository.com/artifact/org.rocksdb/rocksdbjni.

@adamretter
Copy link
Collaborator Author

@jiameixie I am waiting on this one at the moment - #6497

@adamretter
Copy link
Collaborator Author

@jiameixie I just released 5.18.4 to Maven Central (it might take up to 2 hours to appear)

@jiameixie
Copy link

@adamretter That's great! Thanks for your effort.

@Yikun
Copy link
Contributor

Yikun commented Mar 12, 2020

Cool, greate works, and I will also bring this rocksdbjni version to Apache Storm community (Bump the rocksdb from 5.18.3 to 5.18.4) [1]. : )

And looks like the v5.18.4 is still on the road, it doesn't appear in [2] yet.

[1] https://issues.apache.org/jira/browse/STORM-3599
[2] https://mvnrepository.com/artifact/org.rocksdb/rocksdbjni

@adamretter
Copy link
Collaborator Author

@Yikun mvnrepository is only an aggregator. Take a look in Maven Central which is cannonical for RocksJava

alluxio-bot pushed a commit to Alluxio/alluxio that referenced this pull request Feb 8, 2021
Current version of leveldbjni dependency cannot support ARM64 platform,
this change bump the version to 5.18.4 to support ARM64 platform.

A special rocksdbjni version for aarch64 has been released by rocksdb
community. [1][2]
[1] https://github.com/facebook/rocksdb/releases/tag/v5.18.4
[2] facebook/rocksdb#6250

Fix #12818

pr-link: #12819
change-id: cid-a6934d0fcfa4e439a57b2129c942a07e45492c6b
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.

8 participants