-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
removed simplejson #6685
removed simplejson #6685
Conversation
155a367
to
6c02bef
Compare
Thanks for separating this update from #6671. I think this PR is related to #6218. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #6685 +/- ##
=======================================
Coverage 63.30% 63.30%
=======================================
Files 162 162
Lines 13322 13322
Branches 1820 1820
=======================================
Hits 8434 8434
Misses 4599 4599
Partials 289 289
|
@guidopetri Any interest in taking a look at this? 😄 |
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.
Sorry, I don't understand why we're changing to the JSON library. The original thread here doesn't make sense to me - the question of "why are we replacing this with the standard json library?" wasn't answered. Is the standard json library faster?
@guidopetri it was removed in flask 2.0 by default and yes, simplejson is slower. Resultsloads (large obj)json 0.333s dumps (large obj)json 0.364s loads (small objs)json 0.359s dumps (small objs)json 0.542s |
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.
Got it, thanks!
Looks like this PR just needs it's dependencies updated by poetry (again?), then we can merge it. 😄 |
249051f
to
7bd2e00
Compare
Done |
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.
Cool, lets get this merged. 😄
@AndrewChubatiuk Merged, thanks for getting this all done. 😄 |
This PR has made huge regression for double values. I suggest to use orjson. It allows nan, too. In general, it's faster than json and simplejson. You can find many articles easily that orjson is the fastest for most cases. e.g. |
@EugeneChung Sounds like we'd better look into this in more depth. Thanks for pointing out the problem. 😄 |
What type of PR is this?
Removed unneeded simplejson dependency