Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Remove JSON serializer dependency. #54

Merged
merged 1 commit into from
Jul 27, 2022

Conversation

hugopl
Copy link
Collaborator

@hugopl hugopl commented Jul 26, 2022

Liquid::Any is a alias to JSON::Any, but in order to implement Liquid::Drop
the Drop must be part of the Any::Type union, so JSON::Any can't be used anymore.

While "creating" the proper Liquid::Any class I noticed things breaking due to code
that was serializing then deserializing data when setting a context variable, since Any
isn't JSON::Any anymore and serializing/deserialize data is very slow I fixed this as
well, but at the price of flexibility.

i.e.

ctx = Context{"foo" => ["bar"]}

ctx = Context.new
json = ["bar"].to_json
ctx["foo"] = JSON.parse(json)

ctx = Context{"foo" => Any{"bar"}}

To try to be backwards compatible I added few deprecated methods:

  • Context#[]=(String, Array(String))
  • Context#[]=(String, Hash(String, String))

Of course this doesn't cover everything that could be accepted with the JSON serialize/deserialize
approach, but I tried 😁.

Despite of this change be required for a Liquid::Drop implementation, IMO it's better to remove this
JSON hack from code anyway.

`Liquid::Any` is a alias to `JSON::Any`, but in order to implement `Liquid::Drop`
the `Drop` must be part of the `Any::Type` union, so `JSON::Any` can't be used anymore.

While "creating" the proper `Liquid::Any` class I noticed things breaking due to code
that was serializing then deserializing data when setting a context variable, since `Any`
isn't `JSON::Any` anymore and serializing/deserialize data is very slow I fixed this as
well, but at the price of flexibility.

i.e.
```Crystal
ctx = Context{"foo" => ["bar"]}

ctx = Context.new
json = ["bar"].to_json
ctx["foo"] = JSON.parse(json)

ctx = Context{"foo" => Any{"bar"}}
```

To try to be backwards compatible I added few deprecated methods:
- `Context#[]=(String, Array(String))`
- `Context#[]=(String, Hash(String, String))`

Of course this doesn't cover everything that could be accepted with the JSON serialize/deserialize
approach, but I tried 😁.

Despite of this change be required for a `Liquid::Drop` implementation, IMO it's better to remove this
JSON hack from code anyway.
@TechMagister
Copy link

Do we have any measurement of the impact on performance ?

@hugopl
Copy link
Collaborator Author

hugopl commented Jul 26, 2022

Yes, with few gotchas but yes.

           ~~~~ Compact filter ~~~~
to_json/parse 307.44k (  3.25µs) (± 3.26%)  1.46kB/op   9.63× slower
      no json   2.96M (337.79ns) (± 4.16%)    209B/op        fastest
           ~~~~ Split filter ~~~~
to_json/parse 271.72k (  3.68µs) (± 3.99%)  1.72kB/op   5.05× slower
      no json   1.37M (728.15ns) (± 5.32%)    352B/op        fastest

The code used:

require "benchmark"
require "json"

data = Array(JSON::Any).new
data << JSON::Any.new("hey")
data << JSON::Any.new("ho")
data << JSON::Any.new(nil)
data << JSON::Any.new("let's")
data << JSON::Any.new("go")
data << JSON::Any.new(nil)

puts "           ~~~~ Compact filter ~~~~"
Benchmark.ips do |x|
  x.report("to_json/parse") do
    result = data.reject(&.raw.nil?)
    JSON.parse(result.to_json)
  end

  x.report("no json") do
    JSON::Any.new(data.reject(&.raw.nil?))
  end
end

puts "           ~~~~ Split filter ~~~~"
str_data = "hey ho, let's go"
Benchmark.ips do |x|
  x.report("to_json/parse") do
    result = data.reject(&.raw.nil?)
    JSON.parse(str_data.split(" ").to_json)
  end

  x.report("no json") do
    JSON::Any.new(str_data.split(" ").map { |s| JSON::Any.new(s) })
  end
end

The gotchas:

  • I use JSON::Any because Liquid::Any is exactly the same for these constructs used in the benchmark
  • I didn't write a benchmark for Context#[]= because it would be comparing a simple pointer attribution to JSON serialize/parse calls, so the results would be similar to the compact filter... but Context#[]= isn't called often like filters.
  • The result for other filters that does this serialize/parse approach would be similar to these ones.

@hugopl hugopl merged commit 7454579 into amberframework:master Jul 27, 2022
@setanta setanta deleted the remove-json-dep branch September 27, 2022 21:22
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants