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

Performance #89

Open
wants to merge 2 commits into
base: SNAPSHOT
Choose a base branch
from

Conversation

dariuszzbyrad
Copy link
Contributor

  • Use system.arraycopy() method instead of manually copy
  • Use StringBuilder instead of string concatenation in loop

@lessthanoptimal
Copy link
Owner

Thanks for the submision!

StringBuilder updates are likely to be accepted as is.

Did you run any benchmarks for the array copy? One of the things found when EJML was first built is that a lot of common performance advice was wrong or not always true. That's the reason why EJML accesses raw array instead of accessor functions, like every Java book recommends. I'm building it's benchmarks back up using JMH and hopefully there will be an automated process for looking at performance across java versions and platforms. EJML needs to run well on Java 8 to the latest and on Arm 32, 64 and x86-64 processors.

One problem I do notice is that you are using like arraycopy(src,0,src,1,10). That will need to be reverted since it creates a temporary array. One of the design requirements is that we minimize memory creation/destruction. That is a requirement for high performance on small matrices.

From the JavaDoc
If the src and dest arguments refer to the same array object, then the copying is performed as if the components at positions srcPos through srcPos+length-1 were first copied to a temporary array with length components and then the contents of the temporary array were copied into positions destPos through destPos+length-1 of the destination array.

@ennerf
Copy link
Contributor

ennerf commented Sep 25, 2020

The JavaDoc specifies as if, so I think they just use temporary array as an example to describe the behavior rather than actually creating a temporary array.

@lessthanoptimal
Copy link
Owner

Just went to look at the code and it's entirely native. In that case, it won't cause a hit on the GC and a simple microbenchmark should be sufficient. After the threads branch gets merged I'll look into this.

@lessthanoptimal
Copy link
Owner

Could you break the string builder changes off into a separate PR? The array copy will need to be run through micro benchmarks. I've added a lot more JMH benchmarks since this PR was first created. However I don't have tooling in place just yet to easily compare speed across a wide range of JVMs and hardware.

@dariuszzbyrad
Copy link
Contributor Author

Could you break the string builder changes off into a separate PR? The array copy will need to be run through micro benchmarks. I've added a lot more JMH benchmarks since this PR was first created. However I don't have tooling in place just yet to easily compare speed across a wide range of JVMs and hardware.

Yes, sure. I did it. #97

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