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.

Sign up for GitHub

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

Jump to bottom

integrate: Jafama + KMath #357

Merged
merged 7 commits into from
Jun 12, 2021
Merged

integrate: Jafama + KMath #357

merged 7 commits into from
Jun 12, 2021

Conversation

therealansh
Copy link
Contributor

This PR resolves #176.

@CommanderTvis CommanderTvis added this to the 0.3 milestone Jun 2, 2021
@CommanderTvis CommanderTvis self-assigned this Jun 2, 2021
@CommanderTvis CommanderTvis added feature A new feature request performance Performance optimization issue labels Jun 2, 2021
@therealansh therealansh requested a review from CommanderTvis June 2, 2021 20:08
@altavir
Copy link
Member

altavir commented Jun 3, 2021

I've rebased the fork since we do not do merges directly to the master. Please resolve conflicts.

@altavir
Copy link
Member

altavir commented Jun 3, 2021

The module must be better documented (there should be a readme template). Also, there should be examples and a benchmark since the primary aim is performance.

@therealansh
Copy link
Contributor Author

Hey @altavir @CommanderTvis
While running Benchmarks i am running into this problem

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.openjdk.jmh.util.Utils (file:/Users/anshtyagi/.gradle/caches/modules-2/files-2.1/org.openjdk.jmh/jmh-core/1.21/442447101f63074c61063858033fbfde8a076873/jmh-core-1.21.jar) to field java.io.PrintStream.charOut
WARNING: Please consider reporting this to the maintainers of org.openjdk.jmh.util.Utils
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
Exception in thread "main" java.lang.RuntimeException: ERROR: Unable to find the resource: /META-INF/BenchmarkList
	at org.openjdk.jmh.runner.AbstractResourceReader.getReaders(AbstractResourceReader.java:98)
	at org.openjdk.jmh.runner.BenchmarkList.find(BenchmarkList.java:122)
	at org.openjdk.jmh.runner.Runner.internalRun(Runner.java:263)
	at org.openjdk.jmh.runner.Runner.run(Runner.java:209)
	at org.openjdk.jmh.Main.main(Main.java:71)

Process finished with exit code 1

Any workaround to test benchmarks?

@CommanderTvis
Copy link
Collaborator

Use JDK 11. @therealansh

@therealansh
Copy link
Contributor Author

Use JDK 11. @therealansh

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.openjdk.jmh.util.Utils (file:/Users/anshtyagi/.gradle/caches/modules-2/files-2.1/org.openjdk.jmh/jmh-core/1.21/442447101f63074c61063858033fbfde8a076873/jmh-core-1.21.jar) to field java.io.PrintStream.charOut

The issue does not seem to be resolved even after using JDK 11. I am using IntelliJ as IDE and have set the JDK to be 11

@CommanderTvis
Copy link
Collaborator

Use JDK 11. @therealansh

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.openjdk.jmh.util.Utils (file:/Users/anshtyagi/.gradle/caches/modules-2/files-2.1/org.openjdk.jmh/jmh-core/1.21/442447101f63074c61063858033fbfde8a076873/jmh-core-1.21.jar) to field java.io.PrintStream.charOut

The issue does not seem to be resolved even after using JDK 11. I am using IntelliJ as IDE and have set the JDK to be 11

Now it's a warning, and it's an issue to JMH and kotlinx-benchmark, which is already reported AFAIK.

@CommanderTvis
Copy link
Collaborator

@therealansh There are still some conflicts.

altavir
altavir requested changes Jun 6, 2021
View reviewed changes
Copy link
Member

@altavir altavir left a comment

Choose a reason for hiding this comment

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

Benchmark and their results in README are still missing.

kmath-jafama/src/main/kotlin/space/kscience/kmath/jafama/KMathJafama.kt Outdated Show resolved Hide resolved
kmath-jafama/src/main/kotlin/space/kscience/kmath/jafama/KMathJafama.kt Outdated

public override inline fun norm(arg: Double): Double = FastMath.abs(arg)

public override inline fun Double.unaryMinus(): Double = -this
Copy link
Member

Choose a reason for hiding this comment

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

Does not make sense to override those.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in basic arithmetic operations like these library functions are not even used should i just remove them instead?

Copy link
Member

Choose a reason for hiding this comment

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

As long as you implement basic add/multiply it should work. They are present in DoubleField for performance considerations, But I am not sure that they matter.

@therealansh
Copy link
Contributor Author

Benchmark and their results in README are still missing.

As i stated the benchmarks written does not run due to the warning and thus does not produce any results. I'll be finding a work around for it.

@altavir
Copy link
Member

altavir commented Jun 6, 2021

@therealansh The warning is completely harmless. It should work perfectly fine with it. So your problem is with something else. Try looking into the benchmarks subproject and just replicate what happens there.

@CommanderTvis CommanderTvis dismissed their stale review June 6, 2021 13:08

Not actual

@CommanderTvis CommanderTvis removed their assignment Jun 7, 2021
@altavir
Copy link
Member

altavir commented Jun 7, 2021

Still no benchmarcs.

@therealansh
Copy link
Contributor Author

Still no benchmarcs.

The issue is still persistant. The resource file META_INF/BenchmarksList seems to be missing and i have tried the solutions but nothing seems to work

@altavir
Copy link
Member

altavir commented Jun 8, 2021

I am not sure how do you try to add benchmarks, but the ones here are obviously working. You just need to add a dependency on module a appropriate benchmark suite.

@therealansh
Copy link
Contributor Author

I am not sure how do you try to add benchmarks, but the ones here are obviously working. You just need to add a dependency on module a appropriate benchmark suite.

I am trying to run these with JMH but i guess there is an issue with JMH and Kotlin.

@altavir
Copy link
Member

altavir commented Jun 8, 2021

Are you saying that gradle benchmark task is failing? If it is, please create an issue with the description of the problem. The warning you described earlier is not the case of the problem.

@therealansh
Copy link
Contributor Author

@altavir i guess the issue is related to this plugin i guess artyushov/idea-jmh-plugin#22

@altavir
Copy link
Member

altavir commented Jun 8, 2021

Nobody talked about idea plugin. Just run benchmarks from gradle. Please see how it is done for other benchmarks.

@therealansh
Copy link
Contributor Author

Nobody talked about idea plugin. Just run benchmarks from gradle. Please see how it is done for other benchmarks.

I did the run the gradle and build the benchmarks but how can i see the results of the benchmarks?

@altavir
Copy link
Member

altavir commented Jun 8, 2021

In the terminal. Additional documentation is available in the plugin repository.

@therealansh
Copy link
Contributor Author

On running gradle benchmarks it just build but doesnt show any benchmarks of the any of the pre-built benchmark classes (BigIntBenchmark, ArrayBrnchmark and so on) do i have to run any other command as well?

@altavir
Copy link
Member

altavir commented Jun 8, 2021

image

@therealansh
Copy link
Contributor Author

Hey @altavir i added some benchmarks do i need to reflect in Readme as well?

@altavir
Copy link
Member

altavir commented Jun 8, 2021

It would be nice to have the results. In the module readme.

@altavir
Copy link
Member

altavir commented Jun 11, 2021

@therealansh last two thigs before merge:

  • resolve conflicts
  • Add regular Kotlin.math operation to benchmarks for comparison.

@therealansh
Copy link
Contributor Author

@therealansh last two thigs before merge:

  • resolve conflicts
  • Add regular Kotlin.math operation to benchmarks for comparison.

Regarding the conflicts as i said should those basic arithmetic functions be removed? In my opinion they do not serve any purpose when we are comparing the libraries.

@altavir altavir merged commit c1b105d into SciProgCentre:dev Jun 12, 2021
@altavir
Copy link
Member

altavir commented Jun 12, 2021

The benchmark shows that the core kotlin math is actually faster than jafama. But we will merge it nonetheless because it is a good example of a new module.

@therealansh
Copy link
Contributor Author

Is it plausible that jafama might perform better on relatively smaller numbers?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@altavir altavir altavir approved these changes

@CommanderTvis CommanderTvis CommanderTvis left review comments

Assignees
No one assigned
Labels
feature A new feature request performance Performance optimization issue
Projects
None yet
Milestone
0.3
Development

Successfully merging this pull request may close these issues.

Integration with Jafama
3 participants
@therealansh @altavir @CommanderTvis