search

dakrone / cheshire

Clojure JSON and JSON SMILE (binary json format) encoding/decoding
https://github.com/dakrone/cheshire
MIT License
1.49k stars 151 forks source link

Replace `condp` with `case` on `.ordinal` #173

Closed nilern closed 3 years ago

nilern commented 3 years ago

Should be a bit faster (seems like 3%).

borkdude commented 3 years ago

I wonder why *warn-on-reflection* is set to false in project.clj as I wondered if this introduced any reflections (which aren't good for performance and also break GraalVM native-image compilation).

nilern commented 3 years ago

I had set *warn-on-reflection* to true and this does not introduce any reflections (not even at macroexpansion time, which might not even matter).

borkdude commented 3 years ago

@nilern Thanks for checking :)

Have you considered just writing a case + (.getName (class ...)) and dispatching on strings? Imo that would make the code more understandable/less hacky with perhaps the same perf boost.

Although identical? checks should be very fast already.

nilern commented 3 years ago

Have you considered just writing a case + (.getName (class ...)) and dispatching on strings? Imo that would make the code more understandable/less hacky with perhaps the same perf boost.

That sounds more hacky to me and surely it is fastest to dispatch on ints.

borkdude commented 3 years ago

@nilern Fair point. I had trouble parsing what was going on in your macro, but I expanded it using some test examples locally. Clever.

nilern commented 3 years ago

I wish we could just switch on enums directly, like Java. But I was determined to find a way.

dakrone commented 3 years ago

Interestingly, this is slightly slower for benchmarks for me with the merging of #174 (and merging master in to use the latest Jackson dependencies). Is it slower for you with these changes as well?

nilern commented 3 years ago

It does seems to be slower now. Or some benchmarks (t-large-geojson-object) might be 1% faster while others are slower... and some benchmarks are benchmarking roundtrips, which is not useful here. Given such results and the macro cleverness it's probably best to scrap this.

dakrone commented 3 years ago

Sounds good, I'll close this for now, we can always re-open in the future if needed!