Skip to content
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

Merged
merged 24 commits into from Jun 22, 2022

Conversation

pjfanning
Copy link
Member

@pjfanning pjfanning commented Apr 1, 2022

relates to #514

@pjfanning pjfanning marked this pull request as draft April 1, 2022 12:27
assertF2sEquals("9.223404E17", 9.2234038E17f);
assertF2sEquals("6.710887E7", 6.7108872E7f);
//TODO investigate
assertF2sEquals("9.8E-45", 1.0E-44f);
Copy link
Member Author

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?

Copy link

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

@pjfanning
Copy link
Member Author

pjfanning commented Apr 19, 2022

@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?

@cowtowncoder
Copy link
Member

@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 NumberInput there is the question of how to handle configurability; I'm pretty sure it should default to JDK for legacy cases where boolean setting cannot be passed.

@pjfanning
Copy link
Member Author

@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.

@cowtowncoder
Copy link
Member

Ok I think this can be closed?

@pjfanning
Copy link
Member Author

@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),
Copy link
Member

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.

@cowtowncoder
Copy link
Member

@pjfanning d'oh! I really should read the description in detail before commenting something stupid. Yes, that makes sense.
Just let me know when merging would make sense. Could be split in multiple parts too but may not be necessary.

One thing that makes things easier to merge is that 3.0 (master) has immutable factories, so using builders (JsonFactory.builder().... or equivalent from BaseTest) avoids some merging issues. But I can usually handle those quite easily, just cause intermediate merge conflict.

@pjfanning pjfanning changed the title poc of float/double writer fast float/double writer option Jun 21, 2022
@pjfanning pjfanning mentioned this pull request Jun 21, 2022
@pjfanning pjfanning marked this pull request as ready for review June 21, 2022 23:53
@pjfanning
Copy link
Member Author

@cowtowncoder is this PR now a manageable size? It will have some merge issues in master but shouldn't be too bad.

@cowtowncoder cowtowncoder merged commit 84dd12a into FasterXML:2.14 Jun 22, 2022
@cowtowncoder cowtowncoder changed the title fast float/double writer option Improve performance of writing floating-point numbers Jun 22, 2022
@cowtowncoder cowtowncoder added the 2.14 Issue planned (at earliest) for 2.14 label Jun 22, 2022
@cowtowncoder cowtowncoder added this to the 2.14.0 milestone Jun 22, 2022
@pjfanning pjfanning deleted the double-writer branch June 22, 2022 08:22
@cowtowncoder cowtowncoder changed the title Improve performance of writing floating-point numbers Improve performance of writing floating-point numbers (Schubfach) Aug 9, 2022
@cowtowncoder cowtowncoder changed the title Improve performance of writing floating-point numbers (Schubfach) Allow use of faster floating-point number serialization (StreamWriteFeature.USE_FAST_DOUBLE_WRITER) Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.14 Issue planned (at earliest) for 2.14
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants