On Mon, 18 Sep 2023 02:59:06 GMT, Shaojin Wen <d...@openjdk.org> wrote:
>> The change code of print fast-path has been deleted, and parse fast-path has >> added support for the pattern "%8.3f". >> >> Where to draw the line of parse fast-path? I have seen patterns that cause >> performance problems, and they can be easily implemented, so I added them. > > Now parse fast-path supports "8.3f", but not "10.3". Because the fast-path > method code size should be less than 325, for JIT inline What I meant is that theoretically we *could* drop the regex code entirely and write a custom parser that specializes every formatter, but that we probably *shouldn't* as this means duplicating a lot of code and we'd likely end up having to maintain both. Exactly which patterns to specialize for is an open question. `"%8.3f"` is common, sure, but so are specifiers like `"%-6d"`. I think it'd be good if we could collect some stats on which specifier patterns are the most common rather than just picking a few at random. I see you added a microbenchmark for yet another happy case, which sort of misses my point about setting micros up to explore the boundaries: the set of microbenchmarks should ideally explore and verify both fast-paths and slow-paths, to show that the benefit of the former is significant while the overhead added to the slow-path is negligible. Adding a `floatFormat2` that does `return "%1.12f".formatted...`, as an example: Name Cnt Base Error Test Error Unit Diff% StringFormat.floatFormat 15 316,133 ± 7,890 170,958 ± 8,231 ns/op 45,9% (p = 0,000*) StringFormat.floatFormat2 15 342,767 ± 4,721 343,748 ± 3,753 ns/op -0,3% (p = 0,506 ) * = significant This verifies that the added overhead is in the noise when the fast-path fail on this test. We don't need to cover every possibility and have an ever-growing set of micros that all just test the fast-path, so I think you can remove the additions and instead adjust one or two of the existing microbenchmarks so that it verifies the slow-path with your PR applied. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/15776#discussion_r1328460325
