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

Use MultiJson to improve JSON performance when possible #106

Merged
merged 1 commit into from
Mar 25, 2013

Conversation

ezkl
Copy link
Contributor

@ezkl ezkl commented Sep 26, 2012

This pull-request is a response to #99. It simply replaces OkJson w/ Intridea's MultiJson.

From the README:

MultiJSON tries to have intelligent defaulting. That is, if you have any of the supported engines already loaded, it will utilize them before attempting to load any. When loading, libraries are ordered by speed. First Oj, then Yajl, then the JSON gem, then JSON pure. If no other JSON library is available, MultiJSON falls back to OkJson, a simple, vendorable JSON parser.

As an example, to leverage the awesome Oj, add gem 'oj', '>= 1.3.5' to your app's Gemfile and bundle install. MultiJson will automatically use Oj for QC's JSON processing.

@ryandotsmith I didn't touch the tests. Would you like me to add one to assert that MultiJson is working properly?

@ryandotsmith
Copy link
Contributor

@ezkl if you rebase this, I will merge and cut a new release.

@ryandotsmith
Copy link
Contributor

What do you think of the json parser in ruby's stdlib? Would it be simpler to use it instead?

@arr-ee
Copy link

arr-ee commented Jan 10, 2013

@ryandotsmith no it won't. MultiJson is pretty standard solution now, and the main point is that it can use different backends with no configuration (and degrades to stdlib parser or, in worst case, OkJson).
YAJL is faster than stdlib, OJ is faster than YAJL, some JRuby implementations might be faster than stdlib, so why not to give a choice?

If original author won't show up in foreseeable future and you don't want to rebase/add testcases, let me know, I'd be happy to do that.

Cheers, and thanks for this neat exploitation of Postgres' awesomeness!

@senny
Copy link
Contributor

senny commented Mar 12, 2013

It's been quite some time since we had activity on this ticket. @arr-ee do you think you could take this one over?

@arr-ee
Copy link

arr-ee commented Mar 12, 2013

Yeah, sure. Will try to push it this evening (i'm in UTC+8)

@senny
Copy link
Contributor

senny commented Mar 12, 2013

@arr-ee ❤️

@ezkl
Copy link
Contributor Author

ezkl commented Mar 12, 2013

Oof. Sorry for the delay. Must have missed the previous GH notifications.

@senny
Copy link
Contributor

senny commented Mar 12, 2013

@ezkl thanks, do you think you could add that test-case? Also there are 4 commits attached, could you squash them into a single one so that we have all the changes together?

@@ -20,4 +20,6 @@ Gem::Specification.new do |s|
s.require_paths = %w[lib]

s.add_dependency "pg", "~> 0.14.1"
s.add_dependency "scrolls", "~> 0.2.1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we no longer have a dependency on scrolls is it necessary to introduce it again?

@senny
Copy link
Contributor

senny commented Mar 17, 2013

@ezkl would be great if you could add the test-case. Also can you squash your commits together and push a rebased version?

@ezkl
Copy link
Contributor Author

ezkl commented Mar 25, 2013

@senny I removed the old scrolls dependency, bumped multi_json, and squashed: a995f78

I'll gladly add a test. Do you have a particular concern w/r/t coverage?

@senny
Copy link
Contributor

senny commented Mar 25, 2013

Thanks for the update!

If I'm not mistaken the current tests already exercise your code path. If that's the case I think we don't need an extra test-case.

@ryandotsmith
Copy link
Contributor

👍

senny added a commit that referenced this pull request Mar 25, 2013
Use MultiJson to improve JSON performance when possible
@senny senny merged commit 011284b into QueueClassic:master Mar 25, 2013
senny added a commit that referenced this pull request Mar 25, 2013
@senny
Copy link
Contributor

senny commented Mar 25, 2013

@ezkl thanks!

@nashbridges
Copy link

Readme still mentions OkJson.

senny added a commit that referenced this pull request Mar 27, 2013
@senny
Copy link
Contributor

senny commented Mar 27, 2013

@nashbridges thanks for the hint. I updated the README: f251345

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.

5 participants