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

Use JNI directly for posix_fadvise #3824

Merged
merged 4 commits into from
Mar 1, 2023
Merged

Conversation

merlimat
Copy link
Contributor

Motivation

For calling posix_fadvise, we are using JNA which is a wrapper on top of JNI. While JNA is convenient because it's dynamically creating a stub for accessing native methods, it's also very inefficient compared to direct JNI access.

Since we already have JNI modules, we should add posix_fadvise there and remove the dependency on JNA.

@merlimat merlimat added this to the 4.16.0 milestone Feb 28, 2023
Copy link
Contributor

@hangc0276 hangc0276 left a comment

Choose a reason for hiding this comment

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

Great job!

@hangc0276
Copy link
Contributor

Please fix the failed check:

Error:  /home/runner/work/bookkeeper/bookkeeper/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/PageCacheUtil.java:37:28: Name 'FADVISE_POSSIBLE' must match pattern '^[a-z][a-zA-Z0-9]*_?$'. [StaticVariableName]
Audit done.
[INFO] There is 1 error reported by Checkstyle 9.3 with buildtools/src/main/resources/bookkeeper/checkstyle.xml ruleset.
Error:  src/main/java/org/apache/bookkeeper/util/PageCacheUtil.java:[37,28] (naming) StaticVariableName: Name 'FADVISE_POSSIBLE' must match pattern '^[a-z][a-zA-Z0-9]*_?$'.

@codecov-commenter
Copy link

codecov-commenter commented Mar 1, 2023

Codecov Report

Merging #3824 (6ed509d) into master (284cf96) will increase coverage by 19.04%.
The diff coverage is 47.05%.

@@              Coverage Diff              @@
##             master    #3824       +/-   ##
=============================================
+ Coverage     41.40%   60.45%   +19.04%     
- Complexity     4034     5851     +1817     
=============================================
  Files           473      473               
  Lines         40963    40963               
  Branches       5240     5241        +1     
=============================================
+ Hits          16962    24764     +7802     
+ Misses        22160    13981     -8179     
- Partials       1841     2218      +377     
Flag Coverage Δ
bookie 39.90% <47.05%> (?)
remaining 29.54% <47.05%> (?)
replication 41.38% <47.05%> (-0.03%) ⬇️
tls 20.98% <47.05%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...java/org/apache/bookkeeper/util/PageCacheUtil.java 53.12% <40.00%> (ø)
...a/org/apache/bookkeeper/bookie/JournalChannel.java 62.93% <100.00%> (+2.58%) ⬆️
...g/apache/bookkeeper/proto/WriteEntryProcessor.java 69.01% <0.00%> (-9.86%) ⬇️
...pache/bookkeeper/server/service/BookieService.java 61.76% <0.00%> (-2.95%) ⬇️
...va/org/apache/bookkeeper/proto/BookieProtocol.java 83.57% <0.00%> (-0.72%) ⬇️
...ols/cli/commands/client/LedgerMetaDataCommand.java 0.00% <0.00%> (ø)
...rg/apache/bookkeeper/meta/LedgerMetadataSerDe.java 76.06% <0.00%> (+0.77%) ⬆️
...rg/apache/bookkeeper/client/BookieWatcherImpl.java 56.25% <0.00%> (+0.78%) ⬆️
.../java/org/apache/bookkeeper/client/BookKeeper.java 46.51% <0.00%> (+0.89%) ⬆️
...a/org/apache/bookkeeper/client/LedgerCreateOp.java 53.80% <0.00%> (+0.95%) ⬆️
... and 219 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@merlimat merlimat merged commit fbd18ae into apache:master Mar 1, 2023
@merlimat merlimat deleted the remove-jna branch March 1, 2023 01:52
LOG.info("Unable to link C library. Native methods will be disabled.");
} catch (NoSuchMethodError e) {
LOG.warn("Obsolete version of JNA present; unable to register C library");
nativeIO = new NativeIOImpl();
Copy link
Member

Choose a reason for hiding this comment

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

We should check if nativeIO.posix_fdavise works or not. new NativeIOImpl() din't ensure posix_fdavise work

Copy link
Member

@wenbingshen wenbingshen Mar 2, 2023

Choose a reason for hiding this comment

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

@horizonzy I also found this when I run unit tests on mac os.

We don't need to check whether nativeIO.posix_fdavise is valid, because in use, once an error occurs, we will close posix_fdavise, so we can just check whether the library is normal, this is the error I encountered.

I have submit a PR: #3832

    public static void bestEffortRemoveFromPageCache(int fd, long offset, long len) {
        if (!fadvisePossible || fd < 0) {
            return;
        }
        try {
            NATIVE_IO.posix_fadvise(fd, offset, len, POSIX_FADV_DONTNEED);
        } catch (Exception e) {
            log.warn("Failed to perform posix_fadvise: {}", e.getMessage());
            fadvisePossible = false;
        }
    }

Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
* Use JNI directly for posix_fadvise

* Fixed checkstyle

* fixed checkstyle

* Fixed jni function name
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.

None yet

7 participants