add configs to enable fastdoubleparser

[pjfanning]

#577

• the java 8 change is also part of a separate PR
• probably no need for the char-array and byte-array classes
• need to port over the tests from fastdoubleparser (junit5 based)

[pjfanning]

@cowtowncoder I added 40d9944 to try to start an implementation where the user gets to choose whether to use the JDK double parser (default) or the fast double parser.

This starting point highlights just how much code will ultimately need to change to allow this to be a controllable feature. Do you have any thoughts on whether we should proceed or if this change is worthwhile at all?

It feels like it would be tidier to change NumberInput to be an object instance with configuration and instead of having is a stateless singleton with static methods.

[cowtowncoder]

Ok, some notes/comments: I think that attempts to make NumberInput stateful (configurable) are probably not worth all the hassle -- rather, I think we'd make JsonParser be the entry point since it has configurability already. I guess the tricky part would be anything that jackson-databind calls as those would now need to go via JsonParser API, and methods to add are really not logically something that should be there.

Although, we could consider adding something as alternative to NumberInput (NumberDecoder?) which could serve dual purpose -- something JsonParser has and uses itself, and can also return to whoever wants instance. That way parser API would not have various parse methods but it would expose immutable decoded object?

That sounds like the way I'd go about this, I think. We would probably then start actually deprecating methods in NumberInput as well, to move calls over to the new mechanism.

[pjfanning]

@cowtowncoder - I'll keep NumberInput as is and have modified the 'double' parsing methods to take a boolean and deprecated the original versions of these methods - which default to not using the fast-double-parser

[pjfanning]

@cowtowncoder currently, this code relies on a new JsonParser.Feature.USE_FAST_DOUBLE_PARSER - if you are creating a JsonMapper, how do you set JSON parser features?

I have FasterXML/jackson-databind#3475 which shows what a partial uptake in jackson-databind looks like. NumberDeserializers has some methods that use JsonParser but there are also some methods on NumberDeserializers that are only passed a DeserializationContext. Would I need a jackson-databind DeserializationFeature as well (for enabling USE_FAST_DOUBLE_PARSER)?

[cowtowncoder]

@pjfanning No, it should be possible to both:

1. Use defaults for JsonFactory, construct JsonMapper with it (either constructor or JsonBuilder.builder(factory)....build()
2. Change defaults via ObjectReader, something like ObjectReader r = mapper.readerFor(type).with(feature);

Oh.... but we do have one challenge; JsonParser.Feature is being replaced and split into:

• StreamReadFeature for format-independent features
• JsonReadFeature json-specific features

So the preferred path is via JsonReadFeature but for 2.x still also has to map to a JsonParser.Feature.

The other question is whether this should be dynamic feature or not; in many cases I'd probably suggest not.
If so, the feature should probably go in JsonFactory.Feature but there's bit more work to pass around settings from there.

So I guess starting with combo of JsonReadFeature and backing legacy JsonParser.Feature makes sense and we can tweak it further as necessary.

[pjfanning]

@cowtowncoder I just added JsonReadFeature USE_FAST_DOUBLE_PARSER - would you be able to suggest the best way to test that?

[cowtowncoder]

I think it'd be great to be able to run tests across multiple backends (InputStream, Reader, InputData-backed, async).
Some tests under src/test/java/com/fasterxml/jackson/core/read/ have support for that.
Like NumberParsingTest.

But this would add a new value in matrix (for default vs fast parsing).

It might make sense to add a new tests and first just have a relatively simple tests for float values in Arrays, Objects but iterating over all configurations?

[pjfanning]

@cowtowncoder I added some more 'read' tests. Would you be able to review this PR when you have time?

[pjfanning]

@cowtowncoder is this PR under consideration for the v2.14 release or should I target it at another branch?

[cowtowncoder]

@pjfanning Apologies for being very slow to follow up again.

Yes, I think these can and should be considered still for 2.14. I will try to merge the one must-fix I have, the only thing I know has to go in (databind #3473), which I just completed.
And then will get back to working on backlog, including reviewing your changes.

[cowtowncoder]

Hi @pjfanning! Ok, I had a chance to get back to this. I think it makes sense, but now I am bit worried about merging this to master (3.0). Would it be possible to have another PR, subset of this one, which only:

1. Adds new parser feature
2. Adds skeletal versions with new argument in TextBuffer and NumberInput (or whichever classes had it) but with no difference (yet) in behavior

I could then merge this to master (there'd be conflict since JsonParser.Feature is removed, and other small things like that). And once this is in, the main part could be merged to 2.14 and then I can merge forward with likely no conflicts for this bigger part.

Would this be possible? I will make sure to get smaller PR merged ASAP whenever it gets here.

[pjfanning]

@cowtowncoder I have split out #766. That Pr should readily merge to 2.14 and master. It is the biggest part of this PR. After that is merged, I can break this PR up further into smaller PRs that won't merge as easily but by keeping them small, the merge conflicts should be manageable.

[pjfanning]

@cowtowncoder I rebased this PR and it is fairly small now. Could you merge this and you can use #765 as a guide when you hit a few merge issues when you forward merge this PR to master? You won't need to merge #765 itself - it just shows the changes I had to make when I went through a similar exercise trying to fix the merge conflicts.

[cowtowncoder]

@pjfanning Ok I hope to get this done tonight, or if not, over the weekend.

[cowtowncoder]

@pjfanning I'll merge this now but after thinking about this I think that this makes sense as StreamReadFeature over JsonReadFeature because it is a shared setting for multiple backends, even if first/mostly affecting JSON parsing. But I think all textual backends should follow this, and in fact based on changes might.
So, I can move the feature after merging.

[cowtowncoder]

Ok, done: changed feature to be StreamReadFeature; was able to merge it all in master too (although TBH I guess that whether fast parser is used or not is difficult to verify reliably, would show in performance tests).