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 JSON::Builder instead of to_h.to_json #75

Merged
merged 1 commit into from
Oct 18, 2017

Conversation

veelenga
Copy link
Contributor

@veelenga veelenga commented Oct 17, 2017

This change ships two improvements:

  1. Build json with JSON::Builder is faster than to_h.to_json (because of transitional hash of course):
# ../granite-orm/example.cr

require "benchmark"
require "./src/granite_orm"
require "./src/adapter/sqlite"

class Comment < Granite::ORM::Base
  adapter sqlite
  table_name post_comments
  field name : String
  field body : String
end

s = "string"
c = Comment.new(name: s, body: s)

Benchmark.ips do |x|
  x.report("to_h.to_json") do
    c.to_h.to_json
  end

  x.report("JSON::Builder") do
    c.to_json
  end
end
$ DB_NAME=test.db crystal run example.cr --release                                                                                                               
to_h.to_json 814.53k (  1.23µs) (± 0.98%)  1.49× slower
JSON::Builder   1.22M (822.64ns) (± 1.29%)       fastest
  1. Allow to_json on a collection of model objects, i.e. user.posts.to_json should work now. Before there was a compilation error.

Copy link
Contributor

@migeorge migeorge left a comment

Choose a reason for hiding this comment

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

Nice! I didn't realize the previous version wouldn't work with collections.

This is definitely good to know, I like it!

@elorest
Copy link
Member

elorest commented Oct 18, 2017

@migeorge Pretty sure the old version would have worked with collections in the same way. JSON.parse collection is just calling to_json on every element of the collection. This is just a tiny bit faster.

Copy link
Member

@elorest elorest left a comment

Choose a reason for hiding this comment

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

In most cases we'll be modifying the results of to_h in an active model serializer like way. However this is a bit faster to I'm fine with merging it. Thanks.

@veelenga
Copy link
Contributor Author

veelenga commented Oct 18, 2017

@migeorge Pretty sure the old version would have worked with collections in the same way. JSON.parse collection is just calling to_json on every element of the collection. This is just a tiny bit faster.

Unfortunately, it wouldn't. Object#to_json calls Array#to_json(json : JSON::Builder), which forces to have #to_json(json : JSON::Builder) method on a model instead of a plain #to_json. Result:

c = Comment.new(name: "name", body: "body")
[c, c].to_json

# no overload matches 'Comment#to_json' with type JSON::Builder
# Overloads are:
# - Comment#to_json()
# - Object#to_json(io : IO)
# - Object#to_json()
#
#      each &.to_json(json)

In this way it should be working with to_h:

def to_json(json : JSON::Builder)
  to_h.to_json(json)
end

@veelenga
Copy link
Contributor Author

veelenga commented Oct 18, 2017

In most cases we'll be modifying the results of to_h in an active model serializer like way. However this is a bit faster to I'm fine with merging it. Thanks.

This is not a Crystal way to do things. Instead of allocating the result in memory, Crystal is about to provide equivalent methods in the IO itself to send the data directly to the socket, file, stream, etc... I don't have an alternative design for this, but I think it may be a big performance difference, especially when having models with a huge data...

Copy link
Contributor

@eliasjpr eliasjpr left a comment

Choose a reason for hiding this comment

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

How would this work with associations and nested JSON?

Copy link
Member

@drujensen drujensen left a comment

Choose a reason for hiding this comment

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

Didn't see the nested to_json issue coming. Glad to see the additional tests for collections. Thanks for fixing this.

@drujensen drujensen merged commit 9f5e845 into amberframework:master Oct 18, 2017
@drujensen
Copy link
Member

@eliasjpr oops, just saw you had a question after I merged. Are you referring to a has_many relationship?

If so, we would recommend a custom serializer for that use case.

@veelenga veelenga deleted the feat/improve-to_json branch October 18, 2017 15:05
@veelenga
Copy link
Contributor Author

veelenga commented Oct 18, 2017

@eliasjpr here is an example how this works with associations:

require "./src/granite_orm"
require "./src/adapter/mysql"

# CREATE TABLE users (id BIGINT PRIMARY KEY AUTO_INCREMENT, email CHAR(20), name CHAR(20), created_at DATETIME, updated_at DATETIME);
# CREATE TABLE posts (id BIGINT PRIMARY KEY AUTO_INCREMENT, user_id BIGINT, title CHAR(20), created_at DATETIME, updated_at DATETIME);

class User < Granite::ORM::Base
  adapter mysql

  has_many :posts

  field email : String
  field name : String
  timestamps
end

class Post < Granite::ORM::Base
  adapter mysql

  belongs_to :user

  field title : String
  timestamps
end

u = User.new(id: 1, email: "user1@mail.com", name: "Hero").tap &.save
Post.new(title: "Post1", user_id: u.id).tap &.save
Post.new(title: "Post2", user_id: u.id).tap &.save
Post.new(title: "Post3", user_id: u.id).tap &.save

puts u.to_json # {"email":"user1@mail.com","name":"Hero","created_at":"2017-10-18 18:04:09","updated_at":"2017-10-18 18:04:09"}

puts Post.all.to_json # [{"user_id":1,"title":"Post1","created_at":"2017-10-18 18:04:09","updated_at":"2017-10-18 18:04:09"},{"user_id":1,"title":"Post2","created_at":"2017-10-18 18:04:09","updated_at":"2017-10-18 18:04:09"},{"user_id":1,"title":"Post3","created_at":"2017-10-18 18:04:09","updated_at":"2017-10-18 18:04:09"}]
puts u.posts.to_json  # [{"user_id":1,"title":"Post1","created_at":"2017-10-18 18:04:09","updated_at":"2017-10-18 18:04:09"},{"user_id":1,"title":"Post2","created_at":"2017-10-18 18:04:09","updated_at":"2017-10-18 18:04:09"},{"user_id":1,"title":"Post3","created_at":"2017-10-18 18:04:09","updated_at":"2017-10-18 18:04:09"}]

puts Post.all.last.user.to_json # {"email":"user1@mail.com","name":"Hero","created_at":"2017-10-18 18:12:55","updated_at":"2017-10-18 18:12:55"}

What did you mean saying "nested JSON" ?

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