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

JSON encoder refactor #12183

Merged
merged 7 commits into from Nov 26, 2013
Merged

JSON encoder refactor #12183

merged 7 commits into from Nov 26, 2013

Conversation

@chancancode
Copy link
Member

chancancode commented Sep 9, 2013

Goals

Nice to have

  • Pluggable backend? (so that the encode_json gem can take advantage of that)
  • Improved JSON performance (Benchmark: full stack, raw JSON)
  • A technical explanation/spec for the all the relevant design (here)
  • A user guide article explaining all these JSON related stuff

Misc TODOs

chancancode added 7 commits Nov 15, 2013
This is because the new encoder will no longer support encode_json.
Therefore our only choice is to return `to_i` or `to_s` in
`BigDecimal#as_json`. Since casting a BigDecimal to an integer is
most likely a lossy operation, we chose to encode it as a string.

Support for encoding BigDecimal as a string will return via the
`activesupport-json_encoder` gem.
Got all the tests passing again.

Support for `encode_json` has been removed (and consequently the
ability to encode `BigDecimal`s as numbers, as mentioned in the
previous commit). Install the `activesupport-json_encoder` gem
to get it back.
@chancancode
Copy link
Member Author

chancancode commented Nov 26, 2013

Numers are in!

Full stack benchmark: this uses Discourse's bench.rb which uses apache bench to simulate real requests, so it goes through the whole stack. It's JSON-heavy because they dump the JSON data for the pages in the HTML output to "preload" the Ember data store without an additional HTTP request.

Raw JSON benchmark: this uses the same Discourse seed data generator and calculates the time it takes to dump various data.

Seems like the patched version is faster than both 4.0.1 and current master!

I also started a draft on a document that goes into details on the expected behaviour of the various components. (I swear it's not as scary as it looks!) The current implementation might not be 100% inline with the draft yet, but we can work on it in future PRs.

I do plan to turn that into a higher-level user guide at some point.

@chancancode
Copy link
Member Author

chancancode commented Nov 26, 2013

The old encoder is extracted to here: https://github.com/vanruby/activesupport-json_encoder

jeremy added a commit that referenced this pull request Nov 26, 2013
JSON encoder refactor
@jeremy jeremy merged commit f7e4e37 into rails:master Nov 26, 2013
1 check passed
1 check passed
default The Travis CI build passed
Details
@nixme
Copy link

nixme commented Dec 1, 2013

@chancancode I just want to say thank you so much for this work 👏

chancancode added a commit to chancancode/rails that referenced this pull request Dec 3, 2013
@chancancode
Copy link
Member Author

chancancode commented Dec 3, 2013

@nixme 💛

fluxusfrequency added a commit to fluxusfrequency/rails that referenced this pull request Dec 4, 2013
@defp
Copy link

defp commented Apr 9, 2014

👍

@vipulnsward

This comment has been minimized.

Copy link
Member

vipulnsward commented on 80e7552 Apr 11, 2014

Seeing this now. Thanks @chancancode for these related improvements! <3.

@thedrow
Copy link

thedrow commented Jan 21, 2016

In which version this PR landed in rails?

@chancancode
Copy link
Member Author

chancancode commented Jan 21, 2016

4.1

@thedrow
Copy link

thedrow commented Jan 22, 2016

Thanks!

bgeuken added a commit to bgeuken/open-build-service that referenced this pull request Sep 15, 2016
Due to rails/rails#9212 rendering json
caused performance issues and had to be replaced in OBS. We started
to use yajl-ruby instead.
Supposedly this has been solved in rails meanwhile and we can revert
the workaround.

See rails/rails#12183 for details

Partial revert of 148b7a9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.