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

More optimizations #560

Merged
merged 3 commits into from
Jan 1, 2025
Merged

Conversation

alexander-yakushev
Copy link
Contributor

@alexander-yakushev alexander-yakushev commented Dec 29, 2024

Here are some more easy optimizations discovered while benchmarking Metabase's usage of HoneySQL.

  1. clojure.string/split uses the regex machinery which is generally more wasteful than searching for a substring manually. HoneySQL so far only looks for . when splitting names. Besides, plenty of names don't have . in them and they still pay the regex tax. The new splitter function allocates nothing in such case.
  2. Clojure's Keyword object already contains a corresponding Symbol, so we can return it in kw->sym (if there is no namespace). We could actually just call (symbol kw) for the same effect, but I think this behavior was introduced in Clojure 1.10, and HoneySQL claims to support 1.9.
  3. I again tried to address the into's transient-persistent roundtrip. This time, I'm not making any speculative and potentially damaging changes. There are a lot of places in the code where into is used multiple times in succession, hence into* (name is up to be changed) both makes the code cleaner and prevents unnecessary transient roundtrips inbetween.

@alexander-yakushev
Copy link
Contributor Author

Looks like (.sym kw) doesn't fly with Babashka. Perhaps, @borkdude may chime in if this is intentional?

@borkdude
Copy link
Contributor

@alexander-yakushev You can add this:

(:bb (symbol (name k)))

before the :clj branch

@borkdude
Copy link
Contributor

Btw since clojure 1.10 or so (symbol kwd) also just works I believe, so dropping clojure 1.9.0 support may also help

@alexander-yakushev
Copy link
Contributor Author

Yes, I mentioned it in the PR. I'm not sure if this optimization is a good reason to drop 1.9 if there are no others.

@alexander-yakushev
Copy link
Contributor Author

The custom :bb feature flag worked, thanks!

@seancorfield
Copy link
Owner

Given 1.9 usage has dropped to just over 2.5%, I am considering dropping support for it. I'll ask in a few places to see how badly that would inconvenience folks...

@seancorfield seancorfield merged commit 94fae34 into seancorfield:develop Jan 1, 2025
5 checks passed
@alexander-yakushev alexander-yakushev deleted the opt branch January 1, 2025 19:06
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