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

Optimized Map/Set lookups in Structure #1246

Merged

Conversation

joerg1985
Copy link
Contributor

Reduced the number of Set lookups in Structure.read / Structure.write.
Introduced a new class to replace two entries of a HashMap with one.
This reduces the number of lookups and the initial capacity has been
reduced too.

There is one additional change in line ~363 to avoid a Memory.share()
call if the size of the structure is known.

I could create a benchmark if required.

@joerg1985
Copy link
Contributor Author

Here are some benchmark numbers, i will close and reopen this PR to rerun Travis CI.

Branch Benchmark Mode Cnt Score Error Units
master write_read thrpt 25 268977.759 ± 5716.129 ops/s
structure-less-lookups write_read thrpt 25 307419.387 ± 7757.885 ops/s
public class StructureBenchmark {
    @Structure.FieldOrder({"b0", "b1", "b2", "l0", "l1", "l2", "s0", "s1", "s2"})
    public static class MyStructure extends Structure {

        public byte b0;
        public byte b1;
        public byte b2;

        public long l0;
        public long l1;
        public long l2;

        public String s0;
        public String s1;
        public String s2;
    }
    
    @Benchmark
    public void write_read(Blackhole blackhole, MyState state) {
        MyStructure structure = state.structure;

        structure.s0 = structure.s1 = structure.s2 = "test";

        structure.write();
        structure.read();
        
        structure.s0 = structure.s1 = structure.s2 = null;
        
        structure.write();
        structure.read();

        blackhole.consume(structure);
    }
    
    @State(Scope.Thread)
    public static class MyState {
        MyStructure structure = new MyStructure();
    }
}

@joerg1985 joerg1985 closed this Aug 11, 2020
@joerg1985 joerg1985 reopened this Aug 11, 2020
Copy link
Contributor

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

This looks generally good. I've left one question inline. No need for you to close/reopen to trigger the travis builds, we can also trigger them manually when needed (if it's a single, transient failure not associated with the PR)

src/com/sun/jna/Structure.java Show resolved Hide resolved
@joerg1985
Copy link
Contributor Author

Okay, i updated the PR and left some comments to better understand what the code does.
The benchmark numbers are still good / better than the master.

Branch Benchmark Mode Cnt Score Error Units
structure-less-lookups write_read thrpt 25 302414.960 ± 23849.565 ops/s

@matthiasblaesing
Copy link
Member

Looks good to me. Thanks for investigating and cleaning up. I'll give it a few more day to settle - and if nothing further creeps up or someone beats me to it, I'll merge sometime next week.

@matthiasblaesing
Copy link
Member

Ah - one further request: Could you add an entry for the CHANGES.md file?

Reduced the number of Set lookups in Structure.read / Structure.write.
Introduced a new class to replace two entries of a HashMap with one.
This reduces the number of lookups and the initial capacity has been
reduced too. Structure.clear will now clear the native string to ensure
following Structure.write calls will write them again.
@joerg1985
Copy link
Contributor Author

The latest push added the entry to CHANGES.md

@matthiasblaesing
Copy link
Member

Thank you - pulling this in.

@matthiasblaesing matthiasblaesing merged commit 367f2d3 into java-native-access:master Aug 17, 2020
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.

3 participants