-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Adds long compression methods #3148
Conversation
private final ByteOrder order; | ||
private final CompressionFactory.LongEncodingFormatReader baseReader; | ||
|
||
public BlockLayoutIndexedLongsSupplier(int totalSize, int sizePer, ByteBuffer fromBuffer, ByteOrder order, |
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.
Add javadoc here please
@@ -743,12 +745,13 @@ private LongColumnSerializer setupTimeWriter(final IOPeon ioPeon) throws IOExcep | |||
{ | |||
ArrayList<GenericColumnSerializer> metWriters = Lists.newArrayListWithCapacity(mergedMetrics.size()); | |||
final CompressedObjectStrategy.CompressionStrategy metCompression = indexSpec.getMetricCompressionStrategy(); | |||
final CompressionFactory.LongEncodingFormat metEncoding = indexSpec.getMetricLongEncodingFormat(); |
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 more like a longEncoding
than a metEncoding
(keep in mind we will have long dimensions soon; also, __time
is a long).
serializer.close(); | ||
} | ||
} | ||
} |
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.
Newline @ EOF
@acslk Okay, whew, finally finished reading through :) I wrote some comments on specific areas but the general structure looks good to me. The benchmarks and storage numbers look good too! Other than that, like I said in the previous comment, I do think it makes sense to include "none" compression for floats, minus the probably unnecessary floatEncoding stuff. Anyone else have a chance to take a look at this now that it's been edited a bunch? @xvrl @jon-wei |
Added the float support for "none" compression strategy. It uses the same BlockLayout/EntireLayout pattern as long compression, but does not have any encoding strategy/format. |
@acslk I'm a bit puzzled by some of the benchmark numbers. @gianm As far as making Overall I'm on board with this PR, I haven't had a chance to read the code much yet, but the approach is the description seems reasonable. |
*/ | ||
@JsonCreator | ||
public IndexSpec( | ||
@JsonProperty("bitmap") BitmapSerdeFactory bitmapSerdeFactory, | ||
@JsonProperty("dimensionCompression") String dimensionCompression, | ||
@JsonProperty("metricCompression") String metricCompression | ||
@JsonProperty("metricCompression") String metricCompression, |
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 think the implementation of IndexSpec could be simplified by using the Enums directly in the constructor, instead of Strings, jackson can serialize/deserialize those to/from strings.
SegmentMetadataQuery.AnalysisType is an example using @JsonValue and @JsonCreator for that purpose
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.
it seems that jackson cannot convert input string to uppercase for the enum, so serialize/deserialize need to take string and apply conversion.
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.
Nevermind, added the toString and fromString for the enums and replaced the string with enum values. There seems to be a special handle for Uncompressed strategy for dimension compression, where getStrategy returns null for uncompressed. The caller handles the null as indicator to not use compressedIndexedInt, I think it's doing something similar to none strategy in this PR for performance reasons. I removed the null return value and did the special handling for UNCOMPRESSED strategy instead, this syntax change should not affect anything.
done reviewing the new changes, had a few comments but looks good overall |
@xvrl After some investigation, I believe the reason for the discrepancy is due to performance of different components. For block based compression layout, the total time for reading column is composed of : copy byte buffer block + decompression + read values from byte buffer.
I did some benchmark using the uncompressed strategy, which goes through the same process as lz4 for copy block + read value, with the only difference being no decompression step. The difference in time between lz4 and uncompressed should be the time for decompression. From the benchmarks below, it seems that readContinuous and readSkipping decompression time matches. Note delta strategy is renamed to auto since the strategy could use delta, table, or longs depending on data cardinality and maximum offset.
I'm not sure why lz4 decompression is faster for auto compared to longs even though the compressed size are similar. Perhaps less work is done for auto decompression since the decompressed size is smaller. |
// Run LongCompressionBenchmarkFileGenerator to generate the required files before running this benchmark | ||
|
||
@State(Scope.Benchmark) | ||
@Fork(value = 1) |
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.
Let's make this:
@Fork(jvmArgsPrepend = "-server", value = 1)
so that the JVM runs in "server" mode:
http://www.oracle.com/technetwork/java/whitepaper-135217.html#2
@xvrl yes, that's a good point, looks like @acslk findings indicate that auto+lz4 generally decompresses faster than longs+lz4 but individual values are then read out slower (which explains why full scans are hit-or-miss but filtered scans are generally faster in auto+lz4's favor). I just read over the more recent changes (the float compression stuff and the introduction of "auto" encoding strategy) and they look good to me. We've also been running this on our test cluster for a couple of weeks now and it has been working out there. 👍 from me, any one else want to take one more look? @jon-wei @xvrl |
LGTM, 👍 |
Looks like we have a few +1 and no objections, so I'll merge this as discussed in the dev sync today. thanks everyone! |
{ | ||
return ByteStreams.join( | ||
return ByteSource.concat( |
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 broke batch indexing that uses Guava pre-15
private final String metricCompression; | ||
private final CompressedObjectStrategy.CompressionStrategy dimensionCompression; | ||
private final CompressedObjectStrategy.CompressionStrategy metricCompression; | ||
private final CompressionFactory.LongEncodingStrategy longEncoding; |
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.
Shouldn't longEncoding
be checked in IndexSpec.equals()
and hashCode()
?
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, it should.
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.
Fixed that here: #4353
This PR introduces a few new compression strategies for long values that are not block based (described in #3147). The enum CompressionFormat is added to control the compression procedure, which could call GenericIndexed for block based compression.
The compression format added include delta compression, table compression, and a new uncompressed format. Delta compression finds the smallest value in the segment, and store all values as offsets. Table compression maps unique values and store their id. The offsets and ids are stored by using the smallest number of bits required for the maximum values. For example, if the maximum offset is 200, then all values are stored using 8 bits. The new uncompressed format is the uncompressed format without the header for position of each block and empty bytes at beginning of each block.
Update
After some discussion in #3147 , it's decided that compression strategy, which compress and decompress blocks of data without knowing the data format, and encoding format, which encode data using knowledge of its attributes such as size of each element, should be separated. Users would choose the combination of compression and encoding to use.
The existing compressions including LZF, LZ4, and Uncompressed all count as compression strategy. In addition, this PR included the compression NONE, which indicate no compression strategy should be used, and the values should be stored all together instead of in blocks, since this would allow faster random access. The LZF and Uncompressed strategy are still needed for backward compatibility, but they shouldn't be used for new data.
The encoding format introduced in this PR are Delta, Table, and Longs. Delta and table are described above, while Longs just indicated values are stored as 8 byte longs. Segments created prior to this PR are viewed as compression strategy used + Longs encoding. The strategy for choosing the encoding are auto and longs. Auto strategy scans through the data for its cardinality and maximum offset, and choose the most appropriate format. Longs strategy just always choose longs format.
Below is a benchmark of combinations of all compression strategies and encoding strategies using some generated data sets.
Data sets includes:
Generated files size in kb (5,000,000 values)
Benchmarks:
readContinuous reads every value, while readSkipping skips randomly between 0 - 2000 values on each read
Benchmark on master, lz4 correspond to longs + lz4. There is no major performance difference