uschindler commented on PR #12737: URL: https://github.com/apache/lucene/pull/12737#issuecomment-1789483634
Hi @rmuir, I am fine with both approaches. Let me just create a new PR with some changes for the Constants.java class and a separate pkg private utility class for looking up JVM args. This would allow us to handle and log a warning if for some reason the jdk.management module is not available. RAMUsageEstimator has that code and I will refactor it to separate class. I will also move the current lookup of Client VM via sysprop to Constants.java (a constant like `IS_CLIENT_VM`), so we can refer to it at runtime to figure out if JVM has C2 available. The guards like security exception and all stuf will be handled there, too. The checks in Panama and in this PR can then all be done centralized, so you do not need the crazy reflective code. I plan to add a `static Optional<String> getVMOption(String name)` which returns an empty optional if it does not exist or module system / security manager WTF hides it. After that the logic for the Constants.java can be used here and this PR gets easier. I agree to merge it afterwards, after adding the C2 check. All fine then? I don't want to argue here about pros and cons of enabling FMA by default. My standpoint is just different. We have a lot of code in Lucene which may run slow if you choose wrong garbage collector or buggy Java versions or Htospot/CPU combinations. What happens with our scalar code or PFOR if you run it on a Core i3 of 2012 ?!? We can't prevent all slowdowns. If people see slowdowns on AMD or ARM, they have the "easy option" to diable it by passing Hotspot arguments. This can easily be stated in documentation of Solr or Elasticsearch. People that care about speed need to tune anyways. My problem here is that you add another layer of optimization settings that conflict with the knobs the enduser already has. If you disable FMA by default on ARM or AMD there must be the option to turn it on (e.g., sysprop). About the benchmarks: There is a sligt slowdown and a slight speedup on my opteron. The stddev is not too large, but yes it could be both directions. Unfortunately I acnnot easily test it, because you blocked in this PR for me the option to forcefully enable FMA. And that's my big problem. That's not future proof? Why do you want to block people using Lucene 9.9 to still use it in 2 years where maybe all AMD cpus of modern generation are fast? It is not Lucene's responsibility to question defaults set by the JDK. If FMA is so bad (as you aregure on some CPUs, why does Hotspot not disable it by default? Please let the option open to the user. If Elasticserach is afraid of slowing down their users, they can add `+XX:-UseFMA` to their standard jvm-options.properties file shipped with their distribution. This allows users to turn on/off. I agree that we should not use Panama or FMA if C2 is completely disabled. But for everything else we have correct knobs. This is why I strongly -1 to disable FMA on AMD (and also ARM) by default without the possibility to tun is back on. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
