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

Add nativeIO for madvise on mapped page cache #108

Closed
wants to merge 5 commits into from
Closed

Conversation

dstuebe
Copy link
Contributor

@dstuebe dstuebe commented Nov 10, 2019

Changes:

  • Add c build step for jni
  • Add nativeIO madvise system call wrapper
  • Advise page tables and headers as 'will use'
  • Add option to advise blob pages as 'random'
  • Use mapped pages for writing in virtual pages files

Local benchmarking shows little impact on IO performance from use of madvise - this is good as impact might have been negative. Need to test under memory pressure at high IO in production environment. It would be ideal to develop a more rigorous local test with memory pressure.

The write to mapped change is an unexpected benefit. Need to test carefully in production as we have gone back and forth on this as a best practice. It looks like the latest iteration is a clear win for mapped IO as the benchmark time is cut in half!

See https://stackoverflow.com/a/49500154/2136991 for packaging & loading the dynamic library

@codecov-io
Copy link

codecov-io commented Nov 10, 2019

Codecov Report

Merging #108 into master will decrease coverage by 0.59%.
The diff coverage is 70.58%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #108     +/-   ##
===========================================
- Coverage     89.76%   89.17%   -0.6%     
- Complexity      723      724      +1     
===========================================
  Files            56       57      +1     
  Lines          2688     2725     +37     
  Branches        177      178      +1     
===========================================
+ Hits           2413     2430     +17     
- Misses          191      211     +20     
  Partials         84       84
Impacted Files Coverage Δ Complexity Δ
...va/com/upserve/uppend/blobs/VirtualPageFileIO.java 83.33% <ø> (ø) 20 <0> (ø) ⬇️
...c/main/java/com/upserve/uppend/blobs/FilePage.java 100% <ø> (ø) 5 <0> (ø) ⬇️
src/main/java/com/upserve/uppend/BlockedLongs.java 82.68% <100%> (-0.24%) 52 <1> (-3)
...ava/com/upserve/uppend/AppendOnlyStoreBuilder.java 100% <100%> (ø) 14 <2> (+2) ⬆️
.../java/com/upserve/uppend/cli/CommandBenchmark.java 96.15% <100%> (+0.15%) 6 <0> (ø) ⬇️
.../java/com/upserve/uppend/AppendStorePartition.java 87.82% <100%> (+0.1%) 20 <0> (ø) ⬇️
...c/main/java/com/upserve/uppend/blobs/NativeIO.java 36.66% <36.66%> (ø) 1 <1> (?)
...java/com/upserve/uppend/blobs/VirtualPageFile.java 82.2% <96%> (-0.18%) 61 <12> (+1)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5cbe7f...5392a9b. Read the comment docs.

@dstuebe
Copy link
Contributor Author

dstuebe commented Nov 17, 2019

Got the dylib included in the Jar - but I can't get the build to or load to support multiple targets (linux & mac)
It loads automatically from the jar but only assuming the target platform is mac (dylib not so)
Jitpack build fails - apparently they don't support native build - no gcc/clang?

@dstuebe
Copy link
Contributor Author

dstuebe commented Nov 18, 2019

Frustrated with integration for JNI - difficult to build multiple targets, jitpack does not support at all.

Investigating use of JNR-FFI but use of constants is a bit kludgy investigating adding madvise values there.

@dstuebe dstuebe mentioned this pull request Nov 18, 2019
@dstuebe
Copy link
Contributor Author

dstuebe commented Nov 19, 2019

Closing based on revised implementation using JNR-FFI in #109

@dstuebe dstuebe closed this Nov 19, 2019
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.

4 participants