Skip to content

Performance improvements #573

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

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

alexander-yakushev
Copy link
Contributor

Hi, here are a few improvements I discovered while working on Metabase.

The first commit addresses the following issue. Binding dynamic variables is comparatively quite expensive. honey.sql/format binds a lot of dynvars, but most of them usually don't receive meaningful values. A different approach is possible – prepare a bindings map that would contain only the vars with values that actually have changed, and pass that to with-bindings macro. Here are ad-hoc benchmark results obtained from Metabase tests:

Before
Time per call: 228.36 ms   Alloc per call: 559,342,831b
Time per call: 224.91 ms   Alloc per call: 559,342,813b
Time per call: 228.88 ms   Alloc per call: 559,342,813b

After
Time per call: 205.92 ms   Alloc per call: 452,848,231b
Time per call: 202.64 ms   Alloc per call: 452,848,232b
Time per call: 205.16 ms   Alloc per call: 452,848,229b

So, -20% allocations and -10% execution time.

The second commit ensures that xs passed to format-values is a vector because this function traverses xs multiple times using reduce, and the latter is inefficient for arbitrary seqs. The commit also contains a few minor changes that swap lazy seq processing for transducers.

@seancorfield
Copy link
Owner

This seems to break bb and cljs compatibility.

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.

2 participants