Skip to content

Substantial performance improvements #54

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

Merged
merged 13 commits into from
Sep 6, 2012
Merged

Conversation

rolftimmermans
Copy link
Contributor

Hi,

Jbuilder is awesome. However, it can be a little slow if the JSON that is built is very large. This patch aims to improve the situation.

Before I start explaining the changes, let me stress to anyone reading this that it is important to use a fast JSON serialisation library, such as oj. Please use oj before considering this patch an improvement. Compared to slower libraries it may yield a 10x performance improvement during serialisation.

The baseline benchmark that I used can be found here: https://gist.github.com/3638182 It is a stand-alone method that serialises an object with a couple of attributes and 2 arrays with 100 children each. Each child has 2-4 attributes.

Baseline benchmark (using oj as serialisation library), n = 1000:

without key_format
            user     system      total        real
build   6.720000   0.040000   6.760000 (  6.773519)

with Jbuilder.key_format :camelize => :lower
            user     system      total        real
build  38.680000   0.220000  38.900000 ( 39.039534)

Benchmark with the changes in this pull request, n = 1000:

without key_format
            user     system      total        real
build   3.260000   0.020000   3.280000 (  3.281165)

with Jbuilder.key_format :camelize => :lower
            user     system      total        real
build   3.820000   0.020000   3.840000 (  3.841956)

About twice as fast without Jbuilder.key_format. When using Jbuilder.key_format :camelize => :lower the improvements were almost 10x. When used with Rails the performance improvements will be diluted, but could still be significant. It also does not address the performance problems that occur when using (many) partials. I have ideas how to fix those, but have not validated them yet. They also depend on the changes in this pull request.

Here is a summary of the performance-related changes/commits and their cumulative effect on the benchmark without key formatting:

Avoid calls to method_missing by calling set! (5.631219)
Refactor method_missing to use fewer comparisons. (4.949613)
Avoid many calls to Array#first. (4.438711)
Set value directly, avoiding calls to set! (4.376998)
Avoid creating lambda objects. (4.120978)
Key formatter should cache formatted keys. (3.463397)
Avoid calls to respond_to? when we really need Module#===. (3.371929)
Simplify array! to avoid calls to child! (3.281165)

Most changes are quite small, but there is one significant change that I'd like to discuss. It is the change that leads to the enormous difference when using a key formatter.

Previously, each JSON key was always formatted when used. When iterating over many children, this would result in a lot of duplicate effort. Therefore, I've introduced a KeyFormatter object that will cache known keys and only perform (possibly expensive) formatting if the key isn't known. A new KeyFormatter is instantiated for each root Jbuilder object, and reused for child objects.

As currently implemented however, it leads to a subtle change in functionality. Previously, it was possible to expand on previously set key formats:

json.key_format! :upcase
json.child do |json|
  # The key will be "VALUE"
  json.value "two"
end
json.key_format! :upcase
json.child do |json|
  json.key_format! ->(key) {key + " foo"}
  # The key will be "VALUE foo"
  json.value "two"
end

Except it only worked like that when using Jbuilder directly, i.e. outside a Rails jbuilder template. Inside Rails, the key_format was not even propagated to child elements:

json.key_format! :upcase
json.child do |json|
  # The key will be "value"
  json.value "two"
end
json.key_format! :upcase
json.child do |json|
  json.key_format! ->(key) {key + " foo"}
  # The key will be "value foo"
  json.value "two"
end

This pull request also fixes the fact that the key format is not propagated in Rails (and adding a few tests for the JbuilderTemplate class). However, I did not add the second behaviour: the ability to 'stack' multiple key formatters on top of each other. There were no tests for it and I wasn't sure the behaviour was intentional. It should be possible to add such a feature relatively easy, however.

I fully realise this pull request has many changes in it, and that the slightly different behaviour with regards to key formatting needs to be considered carefully.

Please let me know your opinion(s) on these changes. I would be quite happy to adjust these patches if that means some or all of it can be merged.

@rolftimmermans rolftimmermans mentioned this pull request Sep 5, 2012
@bbhoss
Copy link

bbhoss commented Sep 5, 2012

I'm not qualified to review this, but I'd just like to say thanks for taking the time to do this.

@cmer
Copy link

cmer commented Sep 6, 2012

Thanks so much for this. I really appreciate.

This has to be the best pull request explanation ever. @dhh, please merge this, if anything to thank this kind soul for his hard work!

dhh pushed a commit that referenced this pull request Sep 6, 2012
Substantial performance improvements
@dhh dhh merged commit 1b3b173 into rails:master Sep 6, 2012
klaseskilson referenced this pull request in tkomstadius/bruse Apr 23, 2015
There is a huge potential problem in Jbuilder's performance when
handling partials. Caching things properly should solve this.
The solution used comes from http://goo.gl/uWXFKu and
http://goo.gl/JyWc5b

However, testing this in development doesn't help much, since
caching doesn't work in the same way as in production mode.
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