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

fix(ledger): avoid rocksdb segfault #438

Merged
merged 3 commits into from
Dec 13, 2024
Merged

fix(ledger): avoid rocksdb segfault #438

merged 3 commits into from
Dec 13, 2024

Conversation

dnut
Copy link
Contributor

@dnut dnut commented Dec 13, 2024

A segfault occurs in RocksDB when built in ReleaseSafe. This can be reproduced with a minimal example of code that only depends on the C library. It does not segfault when RocksDB is built using cmake. It does not segfault in any other zig optimize mode.

This PR is a quick fix to avoid the segfault by preventing RocksDB from being built by zig with ReleaseSafe. Whenever ReleaseSafe is specified as the build mode, RocksDB will be built with ReleaseFast. Otherwise it will use the same optimize mode as the build as a whole.

@@ -46,7 +46,11 @@ pub fn build(b: *Build) void {
const curl_dep = b.dependency("curl", dep_opts);
const curl_mod = curl_dep.module("curl");

const rocksdb_dep = b.dependency("rocksdb", dep_opts);
const rocksdb_dep = b.dependency("rocksdb", .{
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine, if building a dependency in a different release mode like this won't also lead to some other issues?

Copy link
Contributor

@dadepo dadepo left a comment

Choose a reason for hiding this comment

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

LGTM as a quick fix.

@dnut dnut enabled auto-merge (squash) December 13, 2024 21:55
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: 66bc65b Previous: b0e35a0 Ratio
benchmarkGossipService(1k_pull_resp_msgs) 6284793 nanos 852137 nanos 7.38
BlockstoreReader.getDataShredsForSlot(_) 155732 nanos 53791 nanos 2.90

This comment was automatically generated by workflow using github-action-benchmark.

@dnut dnut merged commit 738639d into main Dec 13, 2024
10 checks passed
@dnut dnut deleted the dnut/fix/rocksdb-segfault branch December 13, 2024 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants