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

Add jsonapi-serializer gem to benchmark and clean up the file #109

Merged
merged 3 commits into from
Apr 18, 2021

Conversation

mediafinger
Copy link
Contributor

@mediafinger mediafinger commented Apr 16, 2021

Hey, I like your simple gem and that it includes a benchmark with various other gems. I was missing the jsonapi-serializers (formerly known as fast_jsonapi) gem and therefore I've added it. I've also added benchmark/ips and refactored the benchmark file (sorting calls and declarations alphabetically and adding some comments to make it simpler to understand the file).

Ok, now you might wonder: "but jsonapi-serializers formats the output differently" and you would be correct. But as I've done in a few projects before, where some endpoints needed a simple, 'flat' serialization and didn't follow the JSON:API standard, I've overwritten its as_json method to get the same output as with the other serializers.

And it is still fast, almost as fast as alba.


I recommend to review this commit by commit as this will make it easier to understand the changes.

Looking at the final version of benchmark/local.rb might also be easier than trying to understand the diff in which I moved multiple blocks and lines up or down to sort them alphabetically.


benchmark/ips output:

Comparison:
            jbuilder:     4676.4 i/s
                alba:     4666.7 i/s - same-ish: difference falls within error
 jsonapi_same_format:     4433.5 i/s - 1.05x  (± 0.00) slower
         blueprinter:     4250.1 i/s - 1.10x  (± 0.00) slower
             jsonapi:     4089.7 i/s - 1.14x  (± 0.00) slower
       representable:     4073.6 i/s - 1.15x  (± 0.00) slower
                 ams:     3651.2 i/s - 1.28x  (± 0.00) slower
         alba_inline:     3330.4 i/s - 1.40x  (± 0.00) slower
               rails:     2641.7 i/s - 1.77x  (± 0.00) slower

PS: I've got the idea from a tweet of @jgaskins who started to expand the benchmark (but didn't open a PR yet):
jgaskins@ad78703

@codecov
Copy link

codecov bot commented Apr 16, 2021

Codecov Report

Merging #109 (12640c6) into master (f2772c2) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #109   +/-   ##
=======================================
  Coverage   99.53%   99.53%           
=======================================
  Files           7        7           
  Lines         217      217           
=======================================
  Hits          216      216           
  Misses          1        1           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2772c2...12640c6. Read the comment docs.

@mediafinger mediafinger mentioned this pull request Apr 16, 2021
@okuramasafumi
Copy link
Owner

Awesome! I also thought benchmark script should be cleaned up. Thank you!

I know there are a variety of gems that also render JSON:API compatible JSON described in https://github.com/jsonapi-serializer/comparisons
Do you think it's worth adding them? If we add them the benchmark would be even more useful for some people, but it also gets slower to execute.
If they only support JSON:API and cannot serialize into plain JSON, it won't be a fair comparison, though.

@mediafinger
Copy link
Contributor Author

Glad you like it :-)

If they only support JSON:API and cannot serialize into plain JSON, it won't be a fair comparison, though.

I think that as the main issue. In the case of fast_jsonapi / jsonapi-serializer I knew how to make it output plain JSON, as I was using it in several projects. But for the other gems, I would not directly know how or if possible at all.

@okuramasafumi
Copy link
Owner

I see, well I'm merging this now!

@okuramasafumi okuramasafumi merged commit c26a4ec into okuramasafumi:master Apr 18, 2021
@stas
Copy link

stas commented Aug 2, 2021

Related #104 (comment)

Please revert this. 🙇

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.

3 participants