Optimize scanning of string offsets to skip pages we don't need to read #2575
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2575 +/- ##
==========================================
+ Coverage 89.24% 93.38% +4.13%
==========================================
Files 1220 1063 -157
Lines 45297 39371 -5926
==========================================
- Hits 40427 36767 -3660
+ Misses 4870 2604 -2266 ☔ View full report in Codecov by Sentry. |
benjaminwinger
force-pushed
the
string-scan-opt
branch
from
df711a2
to
8dedd93
Compare
|
https://github.com/kuzudb/kuzu/actions/runs/7186804200/job/19573028840?pr=2575 Clang-tidy on macos doesn't seem to recognize that |
Can you share more details on your benchmark? What dataset is used, how large the dictionary is and what are the numbers like? |
I've updated the description to include benchmark details, as well as more details about the change |
src/storage/store/string_column.cpp
Outdated
| } | ||
| return std::ranges::subrange(data.begin() + startPos, data.begin() + endPos); | ||
| } | ||
|
|
||
| void StringColumn::scanValuesToVector(Transaction* transaction, node_group_idx_t nodeGroupIdx, |
There was a problem hiding this comment.
This code block is becoming complex now. Can you write a more detailed comment to summarize the flow of scan values when there are (or aren't) lots of duplication?
src/storage/store/string_column.cpp
Outdated
| @@ -28,6 +28,9 @@ StringColumn::StringColumn(std::string name, std::unique_ptr<LogicalType> dataTy | |||
|
|
|||
| void StringColumn::scanOffsets(Transaction* transaction, const ReadState& state, | |||
| string_offset_t* offsets, uint64_t index, uint64_t numValues, uint64_t dataSize) { | |||
| // TODO(bmwinger): this causes an issue with structs where a read is attempted on an empty chunk | |||
There was a problem hiding this comment.
I don't follow here, what's the connection with structs?
There was a problem hiding this comment.
I think it was that for some reason structs are reading the first value, even if the numValues in the metadata is 0, so the assertion fails for strings stored in a struct. As long as everything is zero, that value just ends up being an empty string, but it shouldn't be being read. I'm not sure why it was just for structs.
There was a problem hiding this comment.
Do you have a test case or example that can reproduce that? It looks like we should fix it now or later, better document it down in an issue?
There was a problem hiding this comment.
Un-commenting that line, it causes only EmptyCreateNodeTest.CreateStructWithWriteTransaction to fail. That said, after creating the node, the string chunk should contain a single string, so maybe it's the metadata that's wrong, something to do with the scan occurring before the commit probably.
benjaminwinger
force-pushed
the
string-scan-opt
branch
6 times, most recently
from
3a8a041
to
d0bb44a
Compare
benjaminwinger
force-pushed
the
string-scan-opt
branch
from
d0bb44a
to
2d2aa22
Compare
Benchmark ResultMaster commit hash:
|
Before, we were scanning all offsets at once to reduce the number of scans done (since with dictionary compression, offsets may be out of order even when doing unfiltered scans, so there's no guarantee that they will be adjacent). That had a bad worst case where for chunks with a large dictionary, you might end up scanning the entire chunk to read just a few pages.
So instead, this scans in batches, calculated by finding the next range of values where no two values are further apart than one page (using the number of values per page calculated from the chunk metadata). It will still scan unneeded values, but no longer scan unneeded pages. This had better performance than scanning only adjacent values.
I'm concerned that this is a little complicated, but it gives a moderate improvement to string scanning performance (roughly 10% on columns with a moderate amount of duplication).
Here are two sets of benchmarks on the LDBC-10 comment table (I did two runs to make it clearer when differences were due to externalities like system noise. On one of them, the two runs are superimposed). Note that these are without clearing OS filesystem caches.
I'd also done some cold runs in the same circumstances on the
locationIP, andcontenttables, and got a speedup of about 7% (only numbers I have to hand are 859ms average compared to 924ms average, for thecontentcolumn with the string IDs; non-string IDs were slightly faster, but still a comparable difference).Oddly, the q05 benchmark (scanning
browserUsed) is faster with this change, which I think is suspicious as thebrowserUsedcolumn only has 5 distinct values and should get no benefit from skipping pages.