On Mon, 1 Jul 2024 06:18:55 GMT, Chen Liang <li...@openjdk.org> wrote:
>> Shaojin Wen has updated the pull request incrementally with one additional >> commit since the last revision: >> >> code style > > As promising as the performance number is, I think we need to ensure two > things: > 1. Correctness: this patch adds a lot of special cases; not sure if the > current test cases already cover all of them. In addition, format is i18n > stuff, which will need extra review besides the review for performance gains. > 2. Validity: the existing benchmarks don't have profile pollution: see > `ReflectionSpeedBenchmark` > https://github.com/openjdk/jdk/blob/d9bcf061450ebfb7fe02b5a50c855db1d9178e5d/test/micro/org/openjdk/bench/java/lang/reflect/ReflectionSpeedBenchmark.java#L291 > where the `Method.invoke` and `Constructor.newInstance` are called to tamper > JIT's profiling, as JIT can conclude that only one format shape is ever used > in your benchmark, which is unlikely in production. You should call > `String.format` and `String.formatted` with varied format strings and > arguments in setup for profile pollution.<br> > An extreme example would be at > https://github.com/openjdk/jdk/pull/14944#issuecomment-1644050455, where > `Arrays.hashCode(Object[])` where every element is an `Integer` is extremely > fast, but slows down drastically once different arrays are passed. Fully agree with the points @liach made above that this needs to be scrutinized for correctness and validity. For example we need to better explore cases where detection for the fast path might be costlier, for example a longer format string with or without some acceptable specifiers early, then something that forces us into the slow path only at the end. ------------- PR Comment: https://git.openjdk.org/jdk/pull/19956#issuecomment-2199478317
