Adjust load order of libmimalloc and libbrpc#202
Conversation
|
Warning Rate limit exceeded@xiexiaoy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughBuild scripts now reorder mongod DT_NEEDED to preload libbrpc then libmimalloc via patchelf; docs add guidance for adjusting load order and LD_PRELOAD. README formatting and sample outputs updated; several config-doc anchor slugs corrected. No exported/public API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant Build as Build script
participant PE as patchelf
participant Bin as mongod (ELF)
participant RT as Runtime loader (ld.so)
Dev->>Build: run build_tarball*.bash
Build->>PE: run --set-rpath (existing)
Build->>PE: remove NEEDED: libmimalloc.so.2, libbrpc.so
Build->>PE: add NEEDED in order: libbrpc.so, libmimalloc.so.2
PE->>Bin: update DT_NEEDED entries
note right of Bin: mongod now encodes libbrpc before libmimalloc
Dev->>RT: execute mongod
RT->>Bin: resolve DT_NEEDED and load libraries in declared order
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches and finishing touches✅ Passed checks (3 passed)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
concourse/scripts/build_tarball.bash (1)
290-295: Consider error handling for patchelf operations.The patchelf commands may fail if the libraries are not currently in the DT_NEEDED entries or if the mongod binary doesn't exist. Consider adding error handling or validation to prevent silent failures.
Apply this diff to add basic error checking:
# Preload libmimalloc and libbrpc at launch. -patchelf --remove-needed libmimalloc.so.2 ${DEST_DIR}/bin/mongod -patchelf --remove-needed libbrpc.so ${DEST_DIR}/bin/mongod -patchelf --add-needed libbrpc.so ${DEST_DIR}/bin/mongod -patchelf --add-needed libmimalloc.so.2 ${DEST_DIR}/bin/mongod +if [ -f ${DEST_DIR}/bin/mongod ]; then + patchelf --remove-needed libmimalloc.so.2 ${DEST_DIR}/bin/mongod || true + patchelf --remove-needed libbrpc.so ${DEST_DIR}/bin/mongod || true + patchelf --add-needed libbrpc.so ${DEST_DIR}/bin/mongod + patchelf --add-needed libmimalloc.so.2 ${DEST_DIR}/bin/mongod +else + echo "Warning: ${DEST_DIR}/bin/mongod not found, skipping library preload" +fidocs/how-to-compile.md (1)
133-137: Consider adding path validation example.The LD_PRELOAD example uses hardcoded paths that may not exist on all systems. Consider mentioning that users should adjust the paths to match their installation.
Apply this diff to make the example more flexible:
If you don't adjust load order, then you must set LD_PRELOAD before run EloqDoc. ```bash -export LD_PRELOAD=/usr/local/lib/libmimalloc.so.2:/usr/lib/libbrpc.so +export LD_PRELOAD=$INSTALL_PREFIX/lib/libmimalloc.so.2:$INSTALL_PREFIX/lib/libbrpc.so +# Or adjust paths based on your system's library locations: +# export LD_PRELOAD=/usr/local/lib/libmimalloc.so.2:/usr/lib/libbrpc.so</blockquote></details> <details> <summary>README.md (1)</summary><blockquote> `189-191`: **Consider consistent list formatting.** The static analysis tool flagged inconsistent list formatting. According to the project's markdown style (based on the error patterns), dash-style lists are preferred over asterisk-style. Apply this diff to use consistent list formatting: ```diff -* Follow [compile tutorial](docs/how-to-compile.md) to learn how to compile EloqDoc-RocksDB and EloqDocRocksDBCloud from scratch. -* Follow [deploy cluster](docs/how-to-deploy-cluster.md) to learn how to deploy an EloqDoc-RocksDBCloud cluster. -* Follow [configuration description](docs/configuration-description.md) to learn major configuration parameters. +- Follow [compile tutorial](docs/how-to-compile.md) to learn how to compile EloqDoc-RocksDB and EloqDocRocksDBCloud from scratch. +- Follow [deploy cluster](docs/how-to-deploy-cluster.md) to learn how to deploy an EloqDoc-RocksDBCloud cluster. +- Follow [configuration description](docs/configuration-description.md) to learn major configuration parameters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(7 hunks)concourse/scripts/build_tarball.bash(1 hunks)concourse/scripts/build_tarball_open.bash(1 hunks)docs/how-to-compile.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
91-91: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
92-92: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
189-189: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
190-190: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
191-191: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
🔇 Additional comments (4)
docs/how-to-compile.md (1)
122-137: Good documentation of library load order requirements.The documentation clearly explains the need for library load order adjustment and provides both the patchelf approach and the LD_PRELOAD fallback. This aligns well with the implementation in the build scripts.
README.md (2)
62-62: LGTM on step formatting improvements.The bold formatting makes the step structure clearer and more readable.
Also applies to: 70-70
68-68: Good addition of download page reference.Providing users with a direct link to the download page improves the user experience.
Also applies to: 124-124
concourse/scripts/build_tarball_open.bash (1)
211-216: Verify DT_NEEDED ordering and packaged libs (concourse/scripts/build_tarball_open.bash:211-216)Patchelf here removes then re-adds libbrpc and libmimalloc to influence DT_NEEDED order; dynamic loader behavior can vary and I could not verify in the sandbox (patchelf/readelf unavailable). Run these locally to confirm DT_NEEDED order and that the .so files exist (replace ${DEST_DIR} with the actual path):
Run locally:
patchelf --print-needed ${DEST_DIR}bin/mongod readelf -d ${DEST_DIR}bin/mongod | grep -A5 'NEEDED' objdump -p ${DEST_DIR}bin/mongod | grep NEEDED find . -type f -name 'libbrpc*.so*' -o -name 'libmimalloc*.so*' -print # to observe runtime load sequence: LD_TRACE_LOADED_OBJECTS=1 ${DEST_DIR}bin/mongod | grep -E 'libbrpc|mimalloc'If DT_NEEDED order is not reliably enforced across target runtimes, consider an alternative (explicit RPATH/RUNPATH, LD_PRELOAD wrapper, or packaging order + SONAME verification).
xiexiaoy
force-pushed
the
doc-fix
branch
2 times, most recently
from
7ca0197 to
c3aad07
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
2 participants
Summary by CodeRabbit
Documentation
Chores