use fastdoubleparser to improve double parsing speed #54
Conversation
|
Thanks @rudygt . On
I'll run this on my laptop tomorrow and confirm if I see the same results |
| @@ -85,7 +86,7 @@ Set<NameState> transitionOn(String valString) { | |||
| boolean fieldValueIsNumeric = false; | |||
| if (hasNumeric.get() > 0) { | |||
| try { | |||
| final double numerically = Double.parseDouble(valString); | |||
| final double numerically = parseDouble(valString); | |||
There was a problem hiding this comment.
would prefer to follow Class.staticMethod(...) pattern here.
There was a problem hiding this comment.
ook, I have changed the import
|
I found the presentation of the benchmark results in the PR a little hard to figure out. My first cut, when exploring the impact of a chance, is just to run the |
I'm looking at making benchmarks more readable and to fail in case regressions sneak in. For this change I think
Looks like this doesn't lead to meaningful difference. Based on the originating paper https://arxiv.org/pdf/2101.11408.pdf seems like we'd see some benefits but I think the rest of the code makes a significant impact. Will be checking in profiler later this week. |
I'm curious: Why numeric? Not necessarily disagreeing but wondering why. |
|
I think the change only comes into play when there are a lot of ComparableNumbers being created, during the transitions on the ByteMachine, I think calling it parsing maybe was confusing because it has nothing to do with the json parsing speed?. |
| @@ -85,7 +86,7 @@ Set<NameState> transitionOn(String valString) { | |||
| boolean fieldValueIsNumeric = false; | |||
| if (hasNumeric.get() > 0) { | |||
| try { | |||
| final double numerically = Double.parseDouble(valString); | |||
| final double numerically = NumberInput.parseDouble(valString); | |||
There was a problem hiding this comment.
Looks like ComparableNumber is the bottleneck. I'm not seeing consistent improvement w this change on my laptop, even when I directly use FastDoubleParser.parseDouble(s).
There was a problem hiding this comment.
Same is true in Quamina BTW. That and the Sprintf.
There was a problem hiding this comment.
@rudygt just catching up back on this thread, do you think its' still worth merging this in?
There was a problem hiding this comment.
sorry for letting it hang, I would like to do some isolated benchmarks but currently on vacation, so if we can keep it open for another 20 days or so, if not feel free to close, and I will re open if my tests reveal good results.
There was a problem hiding this comment.
no worries. I'll leave this open. enjoy your time-off !
|
Hello again @baldawar I think I found the issue, I was not calling the correct method in the first version of the pull. Also I added a more formal benchmark to compare the changes and with java 11 I see good improvements. this is the output of running ComparableNumberBenchmark in my setup, also I did a very similar empirical test as the one we used to validate the fast hex to long changes a while ago in ComparableNumberTest.benchmarkDoubleParsers Let me know if how the numbers look like for you, and from there we could see if it makes sense to merge. Thanks. |
| @@ -139,4 +139,38 @@ public void epocTimestampRangeTest() throws Exception { | |||
| } | |||
| assertTrue(m.isEmpty()); | |||
| } | |||
|
|
|||
| @Test() | |||
| @Ignore() | |||
There was a problem hiding this comment.
I was thinking we dont really need this test merged, my intention was only to validate the new code was performing better, but once we make the change I dont see the need to keep this benchmark at all
There was a problem hiding this comment.
Yup. I don't see value in this test at the moment. Happy to take it out.
There was a problem hiding this comment.
soo I took the test out, also fixe the issue with the import and now I think we are good, Thanks.
There was a problem hiding this comment.
| @State(Scope.Benchmark) | ||
| public class ComparableNumberBenchmark { |
There was a problem hiding this comment.
I'm hesitant on merging this one because it would lead to just one benchmark being disconnected from others. Is it possible to push this into a separate branch? we can merge for this one later, when I've brought rest of the benchmark tests to JMH as well.
There was a problem hiding this comment.
agree, I did backup my changes adding jmh to another branch.
|
I'm fine w merging this after the comments are addressed. If there's any update to the perf numbers on the README, feel free to update them as well. |
| import java.util.Map; | ||
| import java.time.Duration; | ||
| import java.time.Instant; | ||
| import java.util.*; |
|
Gentle reminder to do a minor version bump in |
Description of changes:
as part of the latest updated jackson-databind that recently uptaked the fast double parser into it, we can now take advantage of this to improve our double parsing speed (that we happens very often).
more information about the performance gains from the faster double parser can be found on the original issue at FasterXML/jackson-core#577 (comment)
I can see that in my machine this change helps with the performance, so hopefully we can get some more information from other people results and decide if we want to merge.
thanks!.
Benchmark / Performance:
Original
Changes