-
-
Notifications
You must be signed in to change notification settings - Fork 798
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.
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
Allow use of faster floating-point number serialization (StreamWriteFeature.USE_FAST_DOUBLE_WRITER
)
#749
Conversation
assertF2sEquals("9.223404E17", 9.2234038E17f); | ||
assertF2sEquals("6.710887E7", 6.7108872E7f); | ||
//TODO investigate | ||
assertF2sEquals("9.8E-45", 1.0E-44f); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@plokhotnyuk I took some of the Ryu tests and used them to test Schubfach and this line 92 is one strange diff - the assert works as "1.0E-44f" in Ryu but I had to hack the value here for Schubfach. Any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Results from Schubfach are rounded with exact precision of 2 decimal digits.
Just see exact representation of the float with significant 7:
https://float.exposed/0x00000007
@cowtowncoder the Ryu and Schubfach algorithms don't output exactly the same values as JDK Double.toString - at least for some edge cases and when the values differ, the difference is very small - would there be anyway to have jackson-core NumberOutput support JDK approach as default and to have an option to use Schubfach (which is faster than Ryu) if the user enables a flag? |
@pjfanning I think it is reasonable to have a non-default flag to enable "fast but incorrect" (I think? JDK implementations is, from what I recall, quite precisely compatible well-defined criteria of closest value, round-trupping). As with |
@cowtowncoder I'll probably hold back on doing anything with this writer code until the fast double parser work is merged. I think that is getting close enough to something that could be considered for merging. |
Ok I think this can be closed? |
@cowtowncoder the recent merges affect parsing (and deserialization) - this PR affects serialization - there is still a little bit of work to do on this but it is close enough to being ready |
* | ||
* @since 2.14 | ||
*/ | ||
USE_FAST_DOUBLE_WRITER(false, JsonGenerator.Feature.USE_FAST_DOUBLE_WRITER), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As with reader side, let's move this to more general StreamWriteFeature
as it need not be JSON specific but applicable to most if not all textual format backends.
@pjfanning d'oh! I really should read the description in detail before commenting something stupid. Yes, that makes sense. One thing that makes things easier to merge is that 3.0 (master) has immutable factories, so using builders (JsonFactory.builder().... or equivalent from |
@cowtowncoder is this PR now a manageable size? It will have some merge issues in master but shouldn't be too bad. |
StreamWriteFeature.USE_FAST_DOUBLE_WRITER
)
relates to #514