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

ActiveSupport::JSON and json gem #1026

Closed
ujifgc opened this Issue Jan 26, 2013 · 7 comments

Comments

Projects
None yet
3 participants
@ujifgc
Member

ujifgc commented Jan 26, 2013

I profiled #1025 patch and stumbled upon some very disturbing behavior of activesupport/json.

active_support/core_ext/object/to_json
extends basic classes with internal #to_json method. In the beginning it does this:

# Hack to load json gem first so we can overwrite its to_json.
begin
  require 'json'
rescue LoadError
end

This code always loads the whole json gem (which, we know, has quite big memory footprint of ~12MB) just to ensure that internal AS implementation wins the monkey patch battle.

I patched my local activesupport stack to not load json and tested padrino with it. It passes okay and weights 12MB less on x64 MRI19.

What can we do?

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Jan 26, 2013

Member

Urgh. Good catch. I thought active_support lazy-loads json like it does with all the other engines.

I stumbled on the behaviour of the json gem described in the monkey-patch before and am not happy with it. First of all: the active_support monkeypatch fixes a real problem (jsons core_ext patches are very problematic), but we should avoid loading it in any case.

For the moment (in: this weekend), I'd live with the 12mb for the better feature, but you are right: we need to find a bright idea to not needlessly load json (not that anyone and his mother in the gem world are already doing this... :/). This gets especially harmful if you load something like yajl along with it.

Member

skade commented Jan 26, 2013

Urgh. Good catch. I thought active_support lazy-loads json like it does with all the other engines.

I stumbled on the behaviour of the json gem described in the monkey-patch before and am not happy with it. First of all: the active_support monkeypatch fixes a real problem (jsons core_ext patches are very problematic), but we should avoid loading it in any case.

For the moment (in: this weekend), I'd live with the 12mb for the better feature, but you are right: we need to find a bright idea to not needlessly load json (not that anyone and his mother in the gem world are already doing this... :/). This gets especially harmful if you load something like yajl along with it.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Jan 26, 2013

Member

Okay, I found what's messing with my gem stack. ActiveSupport is actually not dependent on json gem, so it succeeds to load the file only if it's bundled with some other gem. In my case this other gem happens to be dm-types, which is not bundled in default padrino-generated project for datamapper.

Though this issue is not exactly padrino's one, I think the casualty should be addressed in some performance-related chapter of our docs.

Member

ujifgc commented Jan 26, 2013

Okay, I found what's messing with my gem stack. ActiveSupport is actually not dependent on json gem, so it succeeds to load the file only if it's bundled with some other gem. In my case this other gem happens to be dm-types, which is not bundled in default padrino-generated project for datamapper.

Though this issue is not exactly padrino's one, I think the casualty should be addressed in some performance-related chapter of our docs.

@ujifgc ujifgc closed this Jan 26, 2013

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Jan 26, 2013

Member

My bad closing this issue. It persists on edge even without bundled json.

Member

ujifgc commented Jan 26, 2013

My bad closing this issue. It persists on edge even without bundled json.

@ujifgc ujifgc reopened this Jan 26, 2013

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Jan 26, 2013

Member

The problem might be that json is also in standard lib :/.

Member

skade commented Jan 26, 2013

The problem might be that json is also in standard lib :/.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Jan 26, 2013

Member

Possible solution: rollback #1025 and rescue NoMethodError in rendering.rb#L159 patching #to_json method by require 'json' or require 'yajl/json_gem' or require 'active_support/json'. This way we stay lean until we really need to render a Hash or an Array.

Member

ujifgc commented Jan 26, 2013

Possible solution: rollback #1025 and rescue NoMethodError in rendering.rb#L159 patching #to_json method by require 'json' or require 'yajl/json_gem' or require 'active_support/json'. This way we stay lean until we really need to render a Hash or an Array.

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Jan 26, 2013

Member

That would only delay the problem until some time after bootstrap, so such patterns should generally be avoided if a problem is already known. A different implementation might still be a good idea:

Another option would be to bypass ActiveSupport and use MultiJSON directly, which doesn't monkey-patch and also avoids loading the whole set of things in:
https://github.com/rails/rails/blob/master/activesupport/lib/active_support/json/encoding.rb

In my opinion, if the json gem has issues by monkey-patching core-objects in a problematic fashion, its not the role of the abstracting library to rush in and fix that. We should discourage using the gem and recommend better libraries like oj instead.

Member

skade commented Jan 26, 2013

That would only delay the problem until some time after bootstrap, so such patterns should generally be avoided if a problem is already known. A different implementation might still be a good idea:

Another option would be to bypass ActiveSupport and use MultiJSON directly, which doesn't monkey-patch and also avoids loading the whole set of things in:
https://github.com/rails/rails/blob/master/activesupport/lib/active_support/json/encoding.rb

In my opinion, if the json gem has issues by monkey-patching core-objects in a problematic fashion, its not the role of the abstracting library to rush in and fix that. We should discourage using the gem and recommend better libraries like oj instead.

ujifgc added a commit to ujifgc/padrino-framework that referenced this issue Jan 26, 2013

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Jan 26, 2013

Member

I agree with multi_json, @ujifgc can u add in the gemfile a commented:

gem 'yajl-ruby', '~>1.1.0'
gem "oj", "~> 2.0.2"

Explaining that these are faster json parser etc...

Member

DAddYE commented Jan 26, 2013

I agree with multi_json, @ujifgc can u add in the gemfile a commented:

gem 'yajl-ruby', '~>1.1.0'
gem "oj", "~> 2.0.2"

Explaining that these are faster json parser etc...

skade added a commit that referenced this issue Jan 27, 2013

Merge pull request #1029 from ujifgc/multi-json
Replace json codec with agnostic multi_json #1026

@ujifgc ujifgc closed this Jan 28, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment