Skip to content

perf(sql): vectorize avg(short) and improve sum() functions memory usage#6166

Merged
bluestreak01 merged 14 commits intomasterfrom
puzpuzpuz_faster_avg_short
Sep 25, 2025
Merged

perf(sql): vectorize avg(short) and improve sum() functions memory usage#6166
bluestreak01 merged 14 commits intomasterfrom
puzpuzpuz_faster_avg_short

Conversation

@puzpuzpuz
Copy link
Copy Markdown
Contributor

@puzpuzpuz puzpuzpuz commented Sep 20, 2025 •
edited
Loading

Long.MAX_VALUE / Short.MAX_VALUE = 281,483,566,907,400, so using long128 as the accumulator in Rosti avg(short) function was an overkill. Also, using long instead of double as the accumulator in Java avg(short) implementation is also beneficial due to no short-to-double conversion and faster addition instructions.

Also, includes the following:

  • Reduces memory footprint of sum() aggregate functions.
  • Adds SumShortGroupByFunction function. It's more efficient than SumLongGroupByFunction that was previously used for short and byte values since it has no null checks.

Hot execution times with ClickBench dataset on Ryzen 7900x running Ubuntu 24.04:

-- master: 22ms
-- patch: 8ms
SELECT SUM(AdvEngineID), COUNT(*), AVG(ResolutionWidth) FROM hits;

-- master: 21ms
-- patch: 5ms
SELECT AVG(ResolutionWidth) FROM hits;

@puzpuzpuz puzpuzpuz self-assigned this Sep 20, 2025
@puzpuzpuz puzpuzpuz added SQL Issues or changes relating to SQL execution Performance Performance improvements labels Sep 20, 2025
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Sep 20, 2025 •
edited
Loading

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch puzpuzpuz_faster_avg_short

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@puzpuzpuz puzpuzpuz force-pushed the puzpuzpuz_faster_avg_short branch from 73c774a to 23b738e Compare September 20, 2025 19:41
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Sep 20, 2025

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ puzpuzpuz
❌ GitHub Actions - Rebuild Native Libraries

GitHub Actions - Rebuild Native Libraries seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@glasstiger
Copy link
Copy Markdown
Contributor

glasstiger commented Sep 24, 2025

[PR Coverage check]

😍 pass : 138 / 160 (86.25%)

file detail

path covered line new line coverage
🔵 io/questdb/griffin/engine/functions/groupby/SumIntGroupByFunction.java 6 12 50.00%
🔵 io/questdb/griffin/engine/functions/groupby/SumFloatGroupByFunction.java 6 12 50.00%
🔵 io/questdb/griffin/engine/functions/groupby/AvgShortGroupByFunction.java 39 46 84.78%
🔵 io/questdb/griffin/engine/functions/groupby/SumLongGroupByFunction.java 11 12 91.67%
🔵 io/questdb/griffin/engine/groupby/vect/AvgShortVectorAggregateFunction.java 13 14 92.86%
🔵 io/questdb/griffin/engine/functions/groupby/SumShortGroupByFunction.java 32 33 96.97%
🔵 io/questdb/griffin/engine/functions/groupby/AvgShortGroupByFunctionFactory.java 4 4 100.00%
🔵 io/questdb/griffin/engine/functions/groupby/SumShortGroupByFunctionFactory.java 4 4 100.00%
🔵 io/questdb/griffin/engine/functions/groupby/SumLong256GroupByFunction.java 8 8 100.00%
🔵 io/questdb/griffin/engine/groupby/vect/SumShortVectorAggregateFunction.java 2 2 100.00%
🔵 io/questdb/griffin/engine/functions/groupby/SumDoubleGroupByFunction.java 13 13 100.00%

@jerrinot jerrinot self-requested a review September 24, 2025 13:32
@ideoma
Copy link
Copy Markdown
Collaborator

ideoma commented Sep 25, 2025

@puzpuzpuz In the PR title, "vectorize" means using some SIMD, but this change does not change this aspect much; it adds the average, sum short implementations in Java. Perhaps the title should just say it speeds up short aggregate functions?

Otherwise, good find, I'm happy to approve

@puzpuzpuz
Copy link
Copy Markdown
Contributor Author

puzpuzpuz commented Sep 25, 2025

@puzpuzpuz In the PR title, "vectorize" means using some SIMD, but this change does not change this aspect much; it adds the average, sum short implementations in Java. Perhaps the title should just say it speeds up short aggregate functions?

@ideoma the vectorization change is in AvgShortVectorAggregateFunction:

-             final double value = Vect.avgShortAcc(address, frameRowCount, countsAddr + (long) workerId * Misc.CACHE_LINE_SIZE);
+             final long value = Vect.sumShort(address, frameRowCount);

Here, Vect.avgShortAcc() wasn't vectorized and only had a *Vanilla implementation in C++ while Vect.sumShort() has a vectorized impl.

@ideoma
Copy link
Copy Markdown
Collaborator

ideoma commented Sep 25, 2025

Okay, fair enough; there is some vectorization, but it's far more than that.

@puzpuzpuz puzpuzpuz changed the title perf(sql): vectorize avg(short) function perf(sql): vectorize avg(short) and improve sum() functions memory usage Sep 25, 2025
@puzpuzpuz
Copy link
Copy Markdown
Contributor Author

puzpuzpuz commented Sep 25, 2025

Okay, fair enough; there is some vectorization, but it's far more than that.

How about "perf(sql): vectorize avg(short) and improve sum() functions memory usage"? I've changed the title to this one.

@puzpuzpuz
Copy link
Copy Markdown
Contributor Author

puzpuzpuz commented Sep 25, 2025

@ideoma thanks for the review!

@bluestreak01 bluestreak01 merged commit 179f801 into master Sep 25, 2025
39 of 40 checks passed
@bluestreak01 bluestreak01 deleted the puzpuzpuz_faster_avg_short branch September 25, 2025 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ideoma ideoma ideoma approved these changes

@jerrinot jerrinot Awaiting requested review from jerrinot

Assignees

@puzpuzpuz puzpuzpuz

Labels

Performance Performance improvements SQL Issues or changes relating to SQL execution

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@puzpuzpuz @CLAassistant @glasstiger @ideoma @bluestreak01