Skip to content

Use Class#getConstructors to check if the Pointer.class based constructor exists#1108

Closed
bjorndarri wants to merge 1 commit intojava-native-access:masterfrom
bjorndarri:master
Closed

Use Class#getConstructors to check if the Pointer.class based constructor exists#1108
bjorndarri wants to merge 1 commit intojava-native-access:masterfrom
bjorndarri:master

Conversation

@bjorndarri
Copy link
Copy Markdown
Contributor

Instead of using Class#getConstructor(Pointer.class) with try-catch.

Relevant thread in JNA forum

And if you want to dive into the reasons why using exceptions for control flow is inefficient:
The Exceptional Performance of Lil' Exception

…ss#getConstructors and check if the Pointer.class based constructor exists
@matthiasblaesing
Copy link
Copy Markdown
Member

A benchmark comparing the two variants (slighty modified to be benchmarkable) can be found here:
https://github.com/matthiasblaesing/benchmark-structureinstantiation

The numbers I got:
Java 8: https://github.com/matthiasblaesing/benchmark-structureinstantiation/blob/master/src/site/run2
Java 11: https://github.com/matthiasblaesing/benchmark-structureinstantiation/blob/master/src/site/run1

Graphics:
https://github.com/matthiasblaesing/benchmark-structureinstantiation/raw/master/src/site/benchmark-structureinstantiation.ods

My conclusion:

  • There is a slightly reduced performance for the cases where a constructor is present, that takes a Pointer as parameter. There is a pretty huge margin of error, that might mask the real performance numbers.
  • For the no-argument constructor case, the performance nearly doubled.
  • If you need peek performance, provide constructor, that takes a pointer

@neilcsmith-net you might be interested in this.

@neilcsmith-net
Copy link
Copy Markdown
Contributor

@matthiasblaesing thanks for taking a look at that! 👍 Not as bad as I feared - the logic is similar to how the JDK will find the constructor, just not able to use internal data.

Was initially surprised that the no argument constructor wasn't closer in performance, then realised it's having to allocate memory?! Not sure that's possible to workaround while backwards compatible though? For that reason, my opinion for what it's worth is still that this fix shouldn't be made, and the exception should be logged explicitly. But if this makes it in, and it causes a noticeable overhead, I'll rewrite any remaining uses we have of this.

@matthiasblaesing
Copy link
Copy Markdown
Member

I rerun the benchmarks and also covered the option to only use the parameterless constructor. That variant comes at the price, that the structure constructor in that case always allocates memory for the structure and then switches to the provided pointer.
The iteration case (that is the suggested new mode) shows a slight reduction of ops/s for the pointer constructor case, that is well within the margin of error (-2.2% to -1.8%), but shows a massive improvement (82%-109%) for the case where no pointer based constructor is present.
Using only the parameterless constructor is not an option, as it s a massive performance penalty if the structure is based on an existing pointer (performance drops to about 50%). In addition higher native and heap memory (unnecessary allocation in the constructor) would be caused.
So at this point from a performance perspective every structure should implement a Pointer based constructor, but from a stability point of view, I would not cut support for structures without a pointer based constructor.
Long story short: I intend to merge this in the next few days.

@matthiasblaesing
Copy link
Copy Markdown
Member

Merged via 044cd6f. Thank you.

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