Fork me on GitHub

I've added the first cut of cached query generation in -- if you use embedded values, they are all cached as-is (and so you get a different cached version for each different set of parameters); if you use named parameters, you get a single cached version (I think! I need to verify this!). Small queries only see a small speedup (2-3x) but large, complex queries can see speedups of up to 20x so this definitely seems worthwhile (if you're using named parameters!). Please take it for a spin and provide feedback. The tests use a basic cache factory (so every single unique query is cached forever) but you could use any type of cache factory from clojure.core.cache.wrapped.

😍 1

Nice! Can you also cite absolute numbers? It would be interesting to see those too.


It varies dramatically depending on the actual query. There are a couple of timing test cases in that PR if you want to try it yourself locally.


@U04V70XH6 Are you open to some improvements with regards to GraalVM? A small tweak would help reduce the binary size.


The tweak would be to pull this: > ;; prefer requiring-resolve but that's 1.10+ only: > (let [_ (require 'clojure.core.cache.wrapped) > through (resolve 'clojure.core.cache.wrapped/lookup-or-miss)] to the top level and wrap it in a try/catch. So if core.cached is on the classpath and it loads correctly, then you will use it, else you will ignore it. This helps in following ways for GraalVM: 1. GraalVM sees the dependency at compile time so it includes it in the image. 2. Dynamic requires tend to bloat the native image, doing this at the top level prevents this. In general: Preventing calls to require "at runtime" would maybe save some runtime overhead. Here is some more info on that:

Ben Sless20:01:08

This reminds me some time ago I floated an idea of splitting the rendering and parsing parts of honeysql queries where you said there are too many ad-hoc cases for it. I can give it a try if you'd like


@U04V15CAJ Sure, that's only in a PR so I can modify it any old way while it's still in progress. Could you add a comment on the PR explaining exactly what needs to be done there, so that it works correctly in Clojure/Script?


@UK0810AQ2 I've no idea what you mean about "parsing" -- HoneySQL does no parsing?


@U04V70XH6 will do tomorrow

Ben Sless10:01:45

@U04V70XH6 honey SQL parses and renders the SQL string in a single pass, but it should be possible to front load most of the parsing and emit a single function that when taking the parameters will emit the rendered query in an optimized manner


@UK0810AQ2 that's what this pull request provides, essentially - does it not address that for you?


@U04V70XH6 I have a PR coming up to your PR. Old situation:

Testing honey.cache-test
Uncached, simple, embedded
"Elapsed time: 2611.85342 msecs"
Cached, simple, embedded
"Elapsed time: 946.489007 msecs"
Uncached, complex, mixed
"Elapsed time: 2058.002488 msecs"
Cached, complex, mixed
"Elapsed time: 90.428769 msecs"


New situation:

Testing honey.cache-test
Uncached, simple, embedded
"Elapsed time: 2451.831827 msecs"
Cached, simple, embedded
"Elapsed time: 527.828611 msecs"
Uncached, complex, mixed
"Elapsed time: 2098.901274 msecs"
Cached, complex, mixed
"Elapsed time: 58.642879 msecs"

Ben Sless12:01:36

@U04V70XH6 haven't looked too deep into it, but if borkdude's numbers are any indication there's at least another order of magnitude of rather easy* speedups.

Ben Sless13:01:37

I'll need to give it a detailed look, but on JVM carrying a single strong builder instead if calling str multiple times has a significant effect


for the JVM but in babashka it will be slower :/


but yeah that would work

Ben Sless13:01:03

Let's take it in the babashka channel I'm curious


What is cached is the generated vector of SQL string + embedded parameter values. If you use named parameters, placeholders are putting into that vector (instead of the lifted values), and the only work done after the cached vector is retrieved is a single pass over the vector, calling each placeholder with the :params hash map. There's no string manipulation performed in that case. String vs StringBuilder would indeed make the uncached version a lot faster but would substantially complicate the code and make writing extensions much harder as well.


There's always a trade-off when it comes to performance optimizations

Ben Sless06:01:55

Anyway, I'll start with profiling, may be some easy optimizations


@UK0810AQ2 Did you get a chance to do any of that profiling? Just curious.


I'm taking a few days off work at the end of this week so I'll be writing up the documentation for this PR and merging it in and making a release. If anyone has feedback and -- especially -- if you can do some testing of the PR in your environment, the next few days would be a good time to get that in... Thanks!

👍 1
Ben Sless04:01:42

@U04V70XH6 was a bit swamped with work and life but should get to it today (Monday morning here already).

Ben Sless06:01:49

Profiling results are in, bit hard to read because of laziness

Ben Sless06:01:26

Unsurprisingly, most work is iteration and format-expr


Yeah, I'd definitely expect some possible optimizations in walking the data and bashing the strings together (but don't want to switch to StringBuilder per above). I'd be surprised if there's much optimization available in the "fill-in-the-blanks" post-cache processing?

Ben Sless18:01:36

I'll give it a look, too

Ben Sless18:01:46

Constantly calling into is also pretty wasteful, and usually destructured right after

Ben Sless18:01:47

Re string bashing, you can use a portable string builder function, then ideally return a single eduction which would walk the data once and collect into a string

Ben Sless18:01:27

Did some experiments this morning, should be possible

Ben Sless18:01:22

But requires care


Feel free to open issues with the results of any of your analysis/optimizations @UK0810AQ2

👍 1