Using protocols in cheshire.generate #32

Closed
weavejester opened this Issue Nov 20, 2012 · 13 comments

Comments

Projects
None yet
3 participants

It seems to me that the cheshire.custom namespace could be deprecated if a protocol was added to the cheshire.generate namespace.

Is there any reason why protocols aren't used in cheshire.generate? The only reason I can think of is performance, but with type hinting protocols are as fast as calling any Java method, AFAIK.

edit: changed wording to be clearer

Owner

dakrone commented Nov 20, 2012

Yes, macro-unrolling dispatch is about ~26% faster than protocol dispatch.

edit: in cheshire's case, not the general case.

Owner

dakrone commented Nov 20, 2012

If you run the :benchmark tests, compare the Custom, bypass core with custom fields and the Core Benchmarks times to see the difference.

Owner

dakrone commented Nov 20, 2012

Although, now that I think about it, I might be able to make fall-through for cheshire.generate go directly to protocol dispatch, so regular dispatch would still be fast, and custom would only be used when needed.

Owner

dakrone commented Nov 20, 2012

Having great luck with this. I may be able to deprecate the custom namespace after all. I'll keep you posted as it progresses.

Thank you for looking into this.

Owner

dakrone commented Nov 20, 2012

I've just released 5.0.0, which deprecates the cheshire.custom namespace in favor of falling back to protocol dispatch in the event of custom types.

All the add-encoder and remove-encoder stuff has been moved to cheshire.generate.

This was a good idea, it allows us to retain the performance for regular encoding, and seamless custom encoding in the same namespace (now only cheshire.core is needed).

Let me know if you have any problems with it.

dakrone closed this Nov 20, 2012

This was a good idea, it allows us to retain the performance for regular encoding, and seamless custom encoding in the same namespace (now only cheshire.core is needed).

If I'm completely honest, I'm not sure how much of this was luck on my part that this panned out, but I'm glad that you were able to give Cheshire the best of both worlds!

lynaghk commented Nov 30, 2012

The toplevel condp used by generate prevents custom serialization of things that implement Clojure protocols.
E.g., all Clojure records are instances of java.util.Map, so even if I extend my record with the JSONable protocol they will still be printed out using generate-map instead of my custom implementation.

Why is there a condp to do manual dispatch instead of relying on Clojure's protocol dispatch?

Owner

dakrone commented Nov 30, 2012

@lynaghk As I mentioned before, macro unrolling with a condp is about ~26% faster than Clojure's protocol dispatch.

I'll definitely take a look at custom protocol generation and get that fixed, thanks for letting me know about it.

lynaghk commented Nov 30, 2012

@dakrone Yeah, I don't know anything about timing but maybe you can just do a (satisifies? JSONable x) first, then delegate to the condp tree. Thanks for taking the time to check this out.

Owner

dakrone commented Dec 3, 2012

@lynaghk I've committed a fix for this, can you give it a try with Cheshire's master branch and let me know if that'll work for you? If it does, I'll do a released with this fix.

lynaghk commented Dec 4, 2012

Confirmed, this works great. Thanks @dakrone.

(ns foo
  (:require cheshire.core
            [cheshire.generate :refer [JSONable to-json]]))

(defrecord X [a]
  JSONable
  (to-json [this jg]
    (.writeString jg "123")))

(let [x (X. 1)]
  (cheshire.core/generate-string x)) ;;=> "\"123\"" 
Owner

dakrone commented Dec 4, 2012

@lynaghk I've released 5.0.1 with this fix, thanks for bringing it to my attention!

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