* fix(influxd): update xxhash, avoid stringtoslicebyte in cache (#578)
* fix(influxd): update xxhash, avoid stringtoslicebyte in cache
This commit does 3 things:
* it updates xxhash from v1 to v2; v2 includes a assembly arm version of
Sum64
* it changes the cache storer to write with a string key instead of a
byte slice. The cache only reads the key which WriteMulti already has
as a string so we can avoid a host of allocations when converting back
and forth from immutable strings to mutable byte slices. This includes
updating the cache ring and ring partition to write with a string key
* it updates the xxhash for finding the cache ring partition to use
Sum64String which uses unsafe pointers to directly use a string as a
byte slice since it only reads the string. Note: this now uses an
assembly version because of the v2 xxhash update. Go 1.22 included new
compiler ability to recognize calls of Method([]byte(myString)) and not
make a copy but from looking at the call sites, I'm not sure the
compiler would recognize it as the conversion to a byte slice was
happening several calls earlier.
That's what this change set does. If we are uncomfortable with any of
these, we can do fewer of them (for example, not upgrade xxhash; and/or
not use the specialized Sum64String, etc).
For the performance issue in maz-rr, I see converting string keys to
byte slices taking between 3-5% of cpu usage on both the primary and
secondary. So while this pr doesn't address directly the increased cpu
usage on the secondary, it makes cpu usage less on both which still
feels like a win. I believe these changes are easier to review that
switching to a byte slice pool that is likely needed in other places as
the compiler provides nearly all of the correctness checks we need (we
are relying also on xxhash v2 being correct).
* helps #550
* chore: fix tests/lint
* chore: don't use assembly version; should inline
This 2 line change causes xxhash to use a purego Sum64 implementation
which allows the compiler to see that Sum64 only read the byte slice
input which them means is can skip the string to byte slice allocation
and since it can skip that, it should inline all the calls to
getPartitionStringKey and Sum64 avoiding 1 call to Sum64String which
isn't inlined.
* chore: update ci build file
the ci build doesn't use the make file!!!
* chore: revert "chore: update ci build file"
This reverts commit 94be66fde03e0bbe18004aab25c0e19051406de2.
* chore: revert "chore: don't use assembly version; should inline"
This reverts commit 67d8d06c02e17e91ba643a2991e30a49308a5283.
(cherry picked from commit 1d334c679ca025645ed93518b7832ae676499cd2)
* feat: need to update go sum
---------
Co-authored-by: Phil Bracikowski <13472206+philjb@users.noreply.github.com>
(cherry picked from commit 06ab224516)
* chore: add scaffolding for naive solution
* feat: test case scaffolding
* fix: implement check for series key before proceeding
* fix: add validation for ReadSeriesKeyMeasurement usage
* refactor: explicit use of series key len
* feat: add remaining check to index
* feat: add check to remaining files
As the Len function is used as part of the parseSeriesKey, this also needs to be accounted for on the nil return from this function as it is used in different contexts
* feat: expand test cases
* chore: go fmt
* chore: update test failure message
* chore: impl feedback on unnecessary sz checks
* feat: expand test cases
* fix: nil series key check
In both sections for index.go there is a pre-existing length check against the series key which should catch invalid values, perhaps this explains why it hasn't cropped up in the reported panics. For even more safety, we can also skip a nil key because we know that subsequent calls will cause a panic where this key is attempted to be used
* fix: remove nil tags check
A key with no tags is valid, so we should not check for BOTH nil key and tags as a key could be nil, which is invalid, yet still have tags and therefore cause the check to pass which we do not want
* feat: extend test cases from feedback
* fix: extend checks for CompareSeriesKeys
* feat: add nilKeyHandler for shared key checking logic
* fix: logical error in nilKeyHandler
Prior to this, the else was always defaulted to at the end of the conditional branch, which causes unexpected behaviour and a failure of a bunch of tests.
* fix: return tags keep nil data
In a recent change to this, we agreed on a simple name == nil check for the actual data. As a follow on to this, I just realised that we don't actually want to nil back the tags, even if they're not checked, because having no tags is a valid input so we can simply return whatever we were passed unchanged.
* fix: use len == 0 for extra safety
* feat: extra test for blank series key
Some series files which are smaller than the standard
sizes cause SIGBUS in influx_inspect and influxd, because
entry iteration walks onto mapped memory not backed by the
the file. Avoid walking off the end of the file while
iterating series entries in oddly sized files.
closes https://github.com/influxdata/influxdb/issues/24508
Co-authored-by: Geoffrey Wossum <gwossum@influxdata.com>
When using queries like 'select count(_seriesKey) from bigmeasurement`, we
should iterate over the tsi structures to serve the query instead of loading
all the series into memory up front.
Closes#20543
Since all tag sets are materialised to strings before this method
returns, a large number of allocations can be avoided by carefully
resuing buffers and containers.
This commit reduces allocations by about 75%, which can be very
significant for high cardinality workloads.
The benchmark results shown below are for a benchmark that asks for all
series keys matching `tag5=value0'.
name old time/op new time/op delta
Index_ConcurrentWriteQuery/inmem/queries_100000-8 5.66s ± 4% 5.70s ± 5% ~ (p=0.739 n=10+10)
Index_ConcurrentWriteQuery/tsi1/queries_100000-8 26.5s ± 8% 26.8s ±12% ~ (p=0.579 n=10+10)
IndexSet_TagSets/1M_series/inmem-8 11.9ms ±18% 10.4ms ± 2% -12.81% (p=0.000 n=10+10)
IndexSet_TagSets/1M_series/tsi1-8 23.4ms ± 5% 18.9ms ± 1% -19.07% (p=0.000 n=10+9)
name old alloc/op new alloc/op delta
Index_ConcurrentWriteQuery/inmem/queries_100000-8 2.50GB ± 0% 2.50GB ± 0% ~ (p=0.315 n=10+10)
Index_ConcurrentWriteQuery/tsi1/queries_100000-8 32.6GB ± 0% 32.6GB ± 0% ~ (p=0.247 n=10+10)
IndexSet_TagSets/1M_series/inmem-8 3.56MB ± 0% 3.56MB ± 0% ~ (all equal)
IndexSet_TagSets/1M_series/tsi1-8 12.7MB ± 0% 5.2MB ± 0% -59.02% (p=0.000 n=10+10)
name old allocs/op new allocs/op delta
Index_ConcurrentWriteQuery/inmem/queries_100000-8 24.0M ± 0% 24.0M ± 0% ~ (p=0.353 n=10+10)
Index_ConcurrentWriteQuery/tsi1/queries_100000-8 96.6M ± 0% 96.7M ± 0% ~ (p=0.579 n=10+10)
IndexSet_TagSets/1M_series/inmem-8 51.0 ± 0% 51.0 ± 0% ~ (all equal)
IndexSet_TagSets/1M_series/tsi1-8 80.4k ± 0% 20.4k ± 0% -74.65% (p=0.000 n=10+10)
Since partitions slice is not protected under a lock, setting it to
nil while closing causes a race if concurrent calls are accessing
the partitions slice elsewhere. Since partitions do handle
concurrency and Open resets the slice, just leave it as it after
closing.
The series file compaction previously did not snapshot the max
offset before compacting and would keep compacting until it reached
the end of segment file. This caused more entries than expected into
the RHH map and this map gets exponentially slower as it gets close
to full.
This commit ensures that the series file should work appropriately on
32-bit architecturs. It does this by reducing the maximum size of a
series file to 512MB on 32-bit systems, which should be fully
addressable.
It further updates tests so that the series file size can be reduced
further when running many tests in parallel on 32-bit architectures.
WeblWabl
Jack
davidby-influx
Sam Arnold
Edd Robinson
Jacob Marble
Grzegorz Pomykala
Ben Johnson
Jason Wilder