-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Optimize large IN filters on integer data types #23456
Conversation
core/trino-main/src/main/java/io/trino/util/FastutilSetHelper.java
Outdated
Show resolved
Hide resolved
b8ecdf2
to
004c5d3
Compare
core/trino-main/src/main/java/io/trino/util/FastutilSetHelper.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/util/FastutilSetHelper.java
Outdated
Show resolved
Hide resolved
bbecfd4
to
f9082b4
Compare
f9082b4
to
5ce783d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm % comments
.add(BigInteger.valueOf(1)); | ||
// A Set based on a bitmap uses (max - min) / 64 longs | ||
// Create a bitset only if it uses fewer entries than the equivalent hash set | ||
if (range.compareTo(BigInteger.valueOf(Integer.MAX_VALUE)) > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a bitset smaller than the L1 cache size, would it make sense to use bitset even if it uses a little bit more memory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, bitset is indeed faster than open hash set at all L1 cache sizes. However, the gap tends to be much smaller for small sized sets.
I'm also hesitant to increase memory usage because this stuff is unaccounted memory usage. So i'll keep the existing logic for now.
core/trino-main/src/test/java/io/trino/sql/gen/BenchmarkDynamicPageFilter.java
Show resolved
Hide resolved
{ | ||
INT32_RANDOM(INTEGER, (block, r) -> INTEGER.writeLong(block, r.nextInt())), | ||
INT64_RANDOM(BIGINT, (block, r) -> BIGINT.writeLong(block, r.nextLong())), | ||
INT64_FIXED_32K(BIGINT, (block, r) -> BIGINT.writeLong(block, r.nextLong() % 32768)), // LongBitSetFilter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you test smaller ranges than 32K? What ranges do we expect in practice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 32K here is actually just the range of the input values. Narrowing the input range ensures that bitset will be chosen instead of hashset.
Size of set is determined by filterSize
and for that the benchmark params are 100, 1000, 5000
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was asking basically if we expect in practice smaller than 32k value ranges (max - min)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what i've seen in benchmarks and other anecdotal cases, the joined columns tend to be dates/times or some kind of unique id or primary key which are usually in a not super-wide range.
core/trino-main/src/test/java/io/trino/sql/gen/BenchmarkDynamicPageFilter.java
Outdated
Show resolved
Hide resolved
Nice improvement! |
core/trino-main/src/main/java/io/trino/util/FastutilSetHelper.java
Outdated
Show resolved
Hide resolved
return new LongBitSetFilter(values, min, max); | ||
} | ||
|
||
private static boolean isDirectLongComparisonValidType(Type type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have this also in SimplifyContinuousInValues
. I would move it to io.trino.type.TypeUtils
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That method is slightly different, over there we're additionally looking for types where it's possible to get the next consecutive value by just incrementing the underlying long by 1.
I've added a code comment and renamed it to be clearer.
core/trino-main/src/test/java/io/trino/sql/gen/BenchmarkDynamicPageFilter.java
Outdated
Show resolved
Hide resolved
public void setup(long inputRows) | ||
{ | ||
// Pollute the JVM profile | ||
for (DataSet dataSet : ImmutableList.of(DataSet.INT32_RANDOM, DataSet.INT64_FIXED_32K, DataSet.INT64_RANDOM)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not guaranteed to pollute the profile. It depends on various factors, such whether it runs just on the interpreter, C1, C2, etc. A better way to do that is to run with JMH's bulk warmup mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made that change, but in practise I'm not finding the JMH's bulk warmup mode to be a better choice.
Using that causes a multi-fold increase in the runtime of the benchmark because it warms up each test permutation for N iterations for each benchmark, whereas what we want is to warm up each test permutation once and then warm N iterations for only the benchmark.
The other way of manually polluting profile may not be guaranteed to work but in practice I have never found it to not work.
On the other hand, the JMH's bulk warmup mode is always so time consuming that I always find myself manually editing the code to remove this and pollute the profile manually to get a JHM result in reasonable amount of time.
And running JMH remotely is not the solution to this problem, it still takes up too much time.
core/trino-main/src/test/java/io/trino/sql/gen/TestDynamicPageFilter.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/test/java/io/trino/util/TestFastutilSetHelper.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/test/java/io/trino/util/TestFastutilSetHelper.java
Outdated
Show resolved
Hide resolved
01dd275
to
3818a6f
Compare
d788915
to
501034b
Compare
core/trino-main/src/main/java/io/trino/util/FastutilSetHelper.java
Outdated
Show resolved
Hide resolved
Use a bitset based filter instead of a hash set when the range of values is narrow enough. We use bitset only when it would occupy lesser space than the equivalent open hash set. This is useful for making evaluation of dynamic filter more efficient as we often collect large integer sets in dynamic filters. BenchmarkDynamicPageFilter.filterPages (filterSize) (inputDataSet) (nonNullsSelectivity) Mode Cnt Before score After score 100 INT64_FIXED_32K 0.2 thrpt 30 446.174 ? 10.598 449.113 ? 5.323 ops/s 1000 INT64_FIXED_32K 0.2 thrpt 30 407.625 ? 3.139 1379.767 ? 19.318 ops/s 5000 INT64_FIXED_32K 0.2 thrpt 30 426.413 ? 6.485 1254.731 ? 11.685 ops/s
501034b
to
8ef3e03
Compare
Description
Use a bitset based filter instead of a hash set when the range of
values is narrow enough. We use bitset only when it would occupy
lesser space than the equivalent open hash set.
This is useful for making evaluation of dynamic filter more efficient
as we often collect large integer sets in dynamic filters.
Additional context and related issues
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: