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

wip: fast_jsonparser adapter for multi_json #201

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maierru
Copy link

@maierru maierru commented Dec 7, 2020

$ ruby benchmark.rb
Warming up --------------------------------------
                  oj     9.067k i/100ms
Calculating -------------------------------------
                  oj    101.310k (± 4.0%) i/s -      1.016M in  10.042777s
Warming up --------------------------------------
     fast_jsonparser    12.778k i/100ms
Calculating -------------------------------------
     fast_jsonparser    126.754k (± 2.6%) i/s -      1.278M in  10.087938s

@maierru
Copy link
Author

maierru commented Dec 7, 2020

For dump purposes using Oj

@maierru maierru closed this Feb 3, 2021
@Jack12816
Copy link

Jack12816 commented Aug 22, 2022

@maierru What was the issue here? I would love to have a fast_jsonparser adapter here :)

@maierru
Copy link
Author

maierru commented Aug 22, 2022

@maierru What was the issue here? I would love to have a fast_jsonparser adapter here :)

stopped because of this

anilmaurya/fast_jsonparser#20

and this (looks dangerous)

anilmaurya/fast_jsonparser#18

@Jack12816
Copy link

The first issue was fixed and the second issue, too I would argue. So, is there any chance to revive this PR?

@maierru maierru reopened this Aug 22, 2022
@djberg96
Copy link
Contributor

True, the memory issue was unconfirmed, can't hurt to have another adapter. Can the adapters be published separately? This was the faraday route I think, makes maintenance easier.

@sferik
Copy link
Member

sferik commented Mar 12, 2024

Allowing adapters to be released as separate gems would certainly make maintenance of this library much easier.

On the other hand, it would require end users to be aware of MultiJSON and bundle their preferred adapter to get optimal performance, which today they get for free. It's also not clear how we would rank adapters if multiple adapters were bundled. Presumably, either the first or last required adapter would win, whereas today the fastest adapter wins. We’d also need to consider the default adapter that would be used if no other adapter is bundled. Should it be the json gem? That has C bindings that might not work on all platforms. Should it be json_pure? That has no C bindings, but would be a slow default, making MultiJSON a bad option for library authors. There may be solutions to these problems, but I want to make we think them through and discuss them before going down this path.

@djberg96
Copy link
Contributor

@sferik Either the default adapter would be the json from the stdlib, or there would be no default. As for multiple things being loaded, if you follow in Faraday's footsteps, it would be based on whatever you require, e.g. require 'multi_json/oj, require multi_json/json, etc.

For a large project, different dependencies might use different parsers, so it shouldn't be global.

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.

4 participants