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

Upgrade rust-rocksdb from 0.16 to 0.18 #6351

Merged
merged 17 commits into from
Mar 1, 2022
Merged

Conversation

EdvardD
Copy link
Contributor

@EdvardD EdvardD commented Feb 25, 2022

Upgrade rust-rocksdb from 0.16 to 0.18;
Make sse4.1, sse4.2 cpu instructions a requirement for running a node.

Brings a log of compilation warnings while compiling on aarch64 because of this issue: rust-lang/cargo#10439.

Commit blocking 0.16->0.18 switch: 'Make SSE inclusion conditional for target features (#526)'
Performance after the change: https://pastebin.com/z0uu0dyg
Performance in master: https://pastebin.com/ECjLFptg

nikurt and others added 3 commits February 25, 2022 12:49
* chore: Upgrade rust-rocksdb 0.16 to 0.18

* zlib causes a linker failure in librocksdb_sys

* .

* .

* Disable bzip2 feature

* fmt
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Copy link
Collaborator

@nagisa nagisa left a comment

Choose a reason for hiding this comment

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

Target feature changes make sense to me. I'm not sure however if we need to audit the rocksdb changes to see if there's any potential protocol breaking changes that might occur as a result of this upgrade.

@EdvardD
Copy link
Contributor Author

EdvardD commented Feb 25, 2022

@nagisa rocksdb 0.18 was already merged before (notice that I've cherry-picked @nikurt 's commit here and added the needed flags) and reverted because of rocksdb read performance degradation. Now we found the reason behind it so I suppose we should be okay to go.

Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@jakmeier jakmeier left a comment

Choose a reason for hiding this comment

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

Adding my approval for params estimator changes.

.cargo/config.toml Show resolved Hide resolved
Copy link
Contributor Author

@EdvardD EdvardD left a comment

Choose a reason for hiding this comment

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

It turned out to be impossible configuring flags per target (x86_64) so they're passed to rust-rocksdb/librocksdb-sys/build.rs so we do that with [build] unconditionally adding flags for all cpu types. We expect this to not break M1 compilation though.

-Dwarnings flag was moved from Makefile to config.toml so it doesn't override flags specified in the config.

.cargo/config.toml Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
.cargo/config.toml Outdated Show resolved Hide resolved
@EdvardD EdvardD requested a review from mina86 March 1, 2022 15:23
@bowenwang1996 bowenwang1996 merged commit 6534625 into master Mar 1, 2022
@bowenwang1996 bowenwang1996 deleted the rust_rocksdb_0_16_to_0_18 branch March 1, 2022 15:51
mina86 pushed a commit to mina86/nearcore that referenced this pull request Apr 7, 2022
This is commit 6534625 upstream.

Co-authored-by: nikurt <86772482+nikurt@users.noreply.github.com>
Co-authored-by: Bowen Wang <bowenwang1996@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants