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

Add StreamReadFeature.USE_FAST_BIG_NUMBER_PARSER to enable faster BigDecimal, BigInteger parsing #851

Merged
merged 12 commits into from Dec 3, 2022

Conversation

pjfanning
Copy link
Member

@pjfanning pjfanning commented Nov 30, 2022

  • So far, this one feature also affects BigInteger parsing, but I could introduce a 2nd feature if that was preferred
  • The fastdoubleparser Big Integer parser allows hex numbers but jackson-core's JSON parsers block the hex numbers anyway.
    • I've got an open issue with fastdoubleparser team and will circle back when I know if they will address this

@pjfanning pjfanning marked this pull request as draft November 30, 2022 13:43
@cowtowncoder
Copy link
Member

Quick note/question: would this actually use multiple processing threads? (wrt references to parallel processing)

@pjfanning
Copy link
Member Author

pjfanning commented Dec 1, 2022

Quick note/question: would this actually use multiple processing threads? (wrt references to parallel processing)

It uses fork-join tasks to do the parallel processing - I haven't delved too much into the details - maybe there is a ForkJoinPool to run the tasks.

Edit: It uses the common ForkJoinPool - see wrandelshofer/FastDoubleParser#24 (comment)

The parallel support is only enabled if you enable 2 separate features.

@cowtowncoder
Copy link
Member

Ok. I feel quite strongly that a library should not be controlling parallelism here so I think anything that uses thread pools, executors etc should be an extension (not part of core by default): otherwise this could cause issues with things like, say, async processing.
And depending on how things are done by default could lead to exhausting of threads, unexpected memory retention (existing Java threads have rather heavy memory overhead for stack).

So I definitely want to make sure to disable processing done using any other model than single main thread; because the alternative is that threading/parallelism aspects would need to be externally configurable.

@pjfanning
Copy link
Member Author

Ok - I can remove the feature to support parallelism

@cowtowncoder
Copy link
Member

@pjfanning Let's do that for now. Having code that would allow that is fine (esp. assuming it'd be hassle to remove it), and we could consider it as a later add-on. Maybe my thinking evolves here; for now there are some limits I like in libraries, like Jackson being pretty much single-threaded linear, no-surprises component (with minor deviation with use of ThreadLocals for buffer recycling); with nothing affecting threading.

@pjfanning pjfanning changed the title WIP: add USE_FAST_BIG_DECIMAL_PARSER add USE_FAST_BIG_DECIMAL_PARSER feature Dec 1, 2022
@pjfanning
Copy link
Member Author

@cowtowncoder is it ok to merge this? There is a minor issue in fastdoubleparser (see issue description) but I don't think this has a big impact. I'd like to try this change out in jackson-databind, etc. It's ages util the actual v2.15.0 release so I think it's ok to live with the potential that we will need to upgrade fastdoubleparser jar later.

@pjfanning pjfanning marked this pull request as ready for review December 1, 2022 21:12
@cowtowncoder
Copy link
Member

cowtowncoder commented Dec 2, 2022

@pjfanning Ok I could merge this but have couple of minor things to change. Will add notes before -- was first thinking of merging, asking for change but with 2.x->3.0 merge it's probably better to get changes in at once.

On hex numbers, that's I guess mostly not-a-problem as tokenization won't allow those: but would allow conversion from "Stringified" numbers. I guess I'm ok with that too. We have some other places where "from String" conversion allows some non-standard representations just out of convenience of using JDK parsing for Integer and Longs etc.

@cowtowncoder cowtowncoder merged commit 63372c4 into FasterXML:2.15 Dec 3, 2022
@cowtowncoder cowtowncoder changed the title add USE_FAST_BIG_DECIMAL_PARSER feature Add StreamReadFeature.USE_FAST_BIG_DECIMAL_PARSER to enable faster BigDecimal, BigInteger parsing Dec 3, 2022
cowtowncoder added a commit that referenced this pull request Dec 3, 2022
@pjfanning pjfanning deleted the big-dec-parser-uptake branch December 3, 2022 09:24
@pjfanning pjfanning changed the title Add StreamReadFeature.USE_FAST_BIG_DECIMAL_PARSER to enable faster BigDecimal, BigInteger parsing Add StreamReadFeature.USE_FAST_BIG_NUMBER_PARSER to enable faster BigDecimal, BigInteger parsing Dec 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants