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

Consider using oj gem for JSON load/dump #903

Closed
jonhyman opened this Issue Mar 25, 2013 · 16 comments

Comments

Projects
None yet
7 participants
@jonhyman
Copy link
Contributor

jonhyman commented Mar 25, 2013

From #902:

We use Oj in our API (docs); Oj is the fastest JSON library out there for Ruby. When we switched our API to use it instead of the default, we saw a noticeable speed increase in API response times, as our profiler indicated that much time was spent in JSON encoding.

master uses the JSON gem, which is slower than Oj.

@tarcieri

This comment has been minimized.

Copy link
Contributor

tarcieri commented Mar 25, 2013

FWIW, I've had a lot of trouble with Oj. It uses a C extension and therefore doesn't work on JRuby and other versions of Ruby that don't support the C extension API. It also does weird things as far as using both Float and BigDecimal to represent floats from JSON.

@jonhyman

This comment has been minimized.

Copy link
Contributor Author

jonhyman commented Mar 25, 2013

Good point, I didn't think of that with JRuby. What JSON parser do you use? Would it be worth being able to swap out the engine used? My original thought was for Resque to just use MultiJson (since it's what ActiveSupport uses), so any engine you configure for your Rails app will be picked up by Resque.

@tarcieri

This comment has been minimized.

Copy link
Contributor

tarcieri commented Mar 25, 2013

At one point it was using multi_json...

@jonhyman

This comment has been minimized.

Copy link
Contributor Author

jonhyman commented Mar 25, 2013

It is no longer doing so on master.

@tarcieri

This comment has been minimized.

Copy link
Contributor

tarcieri commented Mar 25, 2013

Looks like @steveklabnik removed it in 55daf0d

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Mar 25, 2013

Yes. I want to enable people to pick a parser, but I haven't been particularly happy with multi_json. We shouldn't need a meta-gem to do what amounts to swapping out a constant.

@lukaszx0

This comment has been minimized.

Copy link

lukaszx0 commented Apr 9, 2013

What's excatly wrong with it @steveklabnik (just curious) ?

Is this issue sitll open to dicuss or it's already decided that we're using default json gem, period?

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Apr 9, 2013

It's a bunch of machinery for something that shouldn't be complex.

We should use the JSON gem, but we should let people change it out.

@lukaszx0

This comment has been minimized.

Copy link

lukaszx0 commented Apr 10, 2013

Won't it turn out to be reimplementation of multi_json after some time of maintaining it?

@yaauie

This comment has been minimized.

Copy link
Member

yaauie commented Jun 15, 2013

Something as simple as Resque::Config#coder=(thing_that_responds_to_load_and_dump) should work.

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Jun 16, 2013

Yup.

@yaauie

This comment has been minimized.

Copy link
Member

yaauie commented Jun 22, 2013

@edward is going to #pairwithme on Monday to add Resque::Config#coder=.

@Jacobkg

This comment has been minimized.

Copy link
Contributor

Jacobkg commented Jan 14, 2016

@steveklabnik We are back on MultiJson for 1-x so is there anything left to do for this issue?

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Jan 14, 2016

Maybe not. I would like to get re-antiquated with the problem, since I'm a little out of the gem loop these days. I thought MultiJson wasn't really needed anymore directly, but that knowledge might be out of date. Basically, my feel is still #903 (comment)

In other words, I see this ticket as "investigate keeping functionality while removing multijson if possible" rather than "add multijson to a no-longer existing branch"

@yaauie

This comment has been minimized.

Copy link
Member

yaauie commented Jan 14, 2016

@steveklabnik a while ago I remember you abstracting out the encoder; would this be a suitable place to allow someone to provide a plugin that provides its own encoder that is backed by oj/protomodel/yanl/whatever?

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Jan 14, 2016

Yes, possibly. The details are a bit too paged-out of my brain right this moment, so here's the high level:

  1. I would like to allow people to use their own encoders.
  2. I'm not sure MultiJson is the right way to do that anymore.
  3. I don't know what the exact right way is.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment