[PATCH] D130516: [Support] compression classes
ckissane added inline comments. Comment at: clang/lib/Serialization/ASTWriter.cpp:2003-2004 // consumers will not want its contents. + llvm::compression::CompressionAlgorithm CompressionScheme = + llvm::compression::ZlibCompressionAlgorithm(); + dblaikie wrote: > ckissane wrote: > > dblaikie wrote: > > > Doesn't this cause slicing & end up with the base implementation? > > > > > > (also the base class `CompressionAlgorithm` has no virtual functions, so > > > I'm not sure how this is meant to work - does this code all work? Then I > > > must be missing some things - how does this work?) > > You are correct to observe that this patch does not fully pass around > > pointers to instances of the classes, however, because I don't pass > > pointers and the currently repetitive nature of the compression classes, > > this still functions correctly. > > In short, a follow-up patch (which I will shortly upload) will convert this > > to using class instances and passing those around. > > Including reworking functions throughout llvm-project to take advantage of > > this. > > I am aiming to take this 2 step process to cut down on making an already > > large pass larger. > > Let me know if you have any concerns or ideas. > But I'm not sure how this patch works correctly - wouldn't the line below > (`CompressionScheme.supported()`) call `CompressionAlgorithm::supported()` > which always returns false? good catch Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130516/new/ https://reviews.llvm.org/D130516 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D130516: [Support] compression classes
dblaikie added inline comments. Comment at: clang/lib/Serialization/ASTWriter.cpp:2003-2004 // consumers will not want its contents. + llvm::compression::CompressionAlgorithm CompressionScheme = + llvm::compression::ZlibCompressionAlgorithm(); + ckissane wrote: > dblaikie wrote: > > Doesn't this cause slicing & end up with the base implementation? > > > > (also the base class `CompressionAlgorithm` has no virtual functions, so > > I'm not sure how this is meant to work - does this code all work? Then I > > must be missing some things - how does this work?) > You are correct to observe that this patch does not fully pass around > pointers to instances of the classes, however, because I don't pass pointers > and the currently repetitive nature of the compression classes, this still > functions correctly. > In short, a follow-up patch (which I will shortly upload) will convert this > to using class instances and passing those around. > Including reworking functions throughout llvm-project to take advantage of > this. > I am aiming to take this 2 step process to cut down on making an already > large pass larger. > Let me know if you have any concerns or ideas. But I'm not sure how this patch works correctly - wouldn't the line below (`CompressionScheme.supported()`) call `CompressionAlgorithm::supported()` which always returns false? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130516/new/ https://reviews.llvm.org/D130516 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D130516: [Support] compression classes
ckissane added inline comments. Comment at: clang/lib/Serialization/ASTWriter.cpp:2003-2004 // consumers will not want its contents. + llvm::compression::CompressionAlgorithm CompressionScheme = + llvm::compression::ZlibCompressionAlgorithm(); + dblaikie wrote: > Doesn't this cause slicing & end up with the base implementation? > > (also the base class `CompressionAlgorithm` has no virtual functions, so I'm > not sure how this is meant to work - does this code all work? Then I must be > missing some things - how does this work?) You are correct to observe that this patch does not fully pass around pointers to instances of the classes, however, because I don't pass pointers and the currently repetitive nature of the compression classes, this still functions correctly. In short, a follow-up patch (which I will shortly upload) will convert this to using class instances and passing those around. Including reworking functions throughout llvm-project to take advantage of this. I am aiming to take this 2 step process to cut down on making an already large pass larger. Let me know if you have any concerns or ideas. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130516/new/ https://reviews.llvm.org/D130516 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D130516: [Support] compression classes
MaskRay added a comment. Thanks for experimenting the refactoring. My gut feeling is that - for inheritance `llvm/lib/Support/Compression.cpp` introduces quite a bit of complexity. - BestSpeedCompression/DefaultCompression/BestSizeCompression may be kinda weird. Not all algorithms may need all of three. - this new interface does not make parallel compression/decompression easier. I mentioned this on https://groups.google.com/g/generic-abi/c/satyPkuMisk/m/xRqMj8M3AwAJ > I know the paradox of choice:) Certainly that we should not add a plethora of > bzip2, xz, lzo, brotli, etc to generic-abi. I don't think users are so fond > of using a different format for a slight different read/write/compression > ratio/memory usage need. They want to pick one format which performs well > across a wide variety of workloads. (Adding a new format also introduces > complexity on their build system side. They need to teach DWARF consumers > they use. It's not a small undertaking.) I think for a long time llvm/lib/Support/Compression.cpp will stay with just zlib and zstd. There is a possibility if a llvm-project use case needs an alternative (it needs very strong arguments not using zstd), it has the other approach that not using llvm/Support/Compression.h Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130516/new/ https://reviews.llvm.org/D130516 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D130516: [Support] compression classes
dblaikie added inline comments. Comment at: clang/lib/Serialization/ASTWriter.cpp:2003-2004 // consumers will not want its contents. + llvm::compression::CompressionAlgorithm CompressionScheme = + llvm::compression::ZlibCompressionAlgorithm(); + Doesn't this cause slicing & end up with the base implementation? (also the base class `CompressionAlgorithm` has no virtual functions, so I'm not sure how this is meant to work - does this code all work? Then I must be missing some things - how does this work?) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130516/new/ https://reviews.llvm.org/D130516 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D130516: [Support] compression classes
ckissane updated this revision to Diff 447469.
ckissane added a comment.
- format
- merge fix
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130516/new/
https://reviews.llvm.org/D130516
Files:
clang-tools-extra/clangd/index/Serialization.cpp
clang-tools-extra/clangd/unittests/SerializationTests.cpp
clang/lib/Driver/ToolChains/Clang.cpp
clang/lib/Serialization/ASTReader.cpp
clang/lib/Serialization/ASTWriter.cpp
lld/ELF/Driver.cpp
lld/ELF/InputSection.cpp
llvm/include/llvm/Object/Decompressor.h
llvm/include/llvm/Support/Compression.h
llvm/lib/MC/ELFObjectWriter.cpp
llvm/lib/ObjCopy/ELF/ELFObject.cpp
llvm/lib/Object/Decompressor.cpp
llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
llvm/lib/ProfileData/InstrProf.cpp
llvm/lib/ProfileData/SampleProfReader.cpp
llvm/lib/ProfileData/SampleProfWriter.cpp
llvm/lib/Support/Compression.cpp
llvm/tools/llvm-mc/llvm-mc.cpp
llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
llvm/unittests/ProfileData/InstrProfTest.cpp
llvm/unittests/Support/CompressionTest.cpp
Index: llvm/unittests/Support/CompressionTest.cpp
===
--- llvm/unittests/Support/CompressionTest.cpp
+++ llvm/unittests/Support/CompressionTest.cpp
@@ -22,31 +22,43 @@
namespace {
-#if LLVM_ENABLE_ZLIB
-static void testZlibCompression(StringRef Input, int Level) {
+static void testCompressionAlgorithm(
+StringRef Input, int Level,
+compression::CompressionAlgorithm CompressionScheme,
+std::string ExpectedDestinationBufferTooSmallErrorMessage) {
SmallVector Compressed;
SmallVector Uncompressed;
- zlib::compress(arrayRefFromStringRef(Input), Compressed, Level);
+ CompressionScheme.compress(arrayRefFromStringRef(Input), Compressed, Level);
// Check that uncompressed buffer is the same as original.
- Error E = zlib::uncompress(Compressed, Uncompressed, Input.size());
+ Error E =
+ CompressionScheme.decompress(Compressed, Uncompressed, Input.size());
consumeError(std::move(E));
EXPECT_EQ(Input, toStringRef(Uncompressed));
if (Input.size() > 0) {
// Uncompression fails if expected length is too short.
-E = zlib::uncompress(Compressed, Uncompressed, Input.size() - 1);
-EXPECT_EQ("zlib error: Z_BUF_ERROR", llvm::toString(std::move(E)));
+E = CompressionScheme.decompress(Compressed, Uncompressed,
+ Input.size() - 1);
+EXPECT_EQ(ExpectedDestinationBufferTooSmallErrorMessage,
+ llvm::toString(std::move(E)));
}
}
+#if LLVM_ENABLE_ZLIB
+static void testZlibCompression(StringRef Input, int Level) {
+ testCompressionAlgorithm(Input, Level, ZlibCompressionAlgorithm(),
+ "zlib error: Z_BUF_ERROR");
+}
+
TEST(CompressionTest, Zlib) {
- testZlibCompression("", zlib::DefaultCompression);
+ compression::CompressionAlgorithm CompressionScheme =
+ compression::ZlibCompressionAlgorithm();
+ testZlibCompression("", CompressionScheme.DefaultCompression);
- testZlibCompression("hello, world!", zlib::NoCompression);
- testZlibCompression("hello, world!", zlib::BestSizeCompression);
- testZlibCompression("hello, world!", zlib::BestSpeedCompression);
- testZlibCompression("hello, world!", zlib::DefaultCompression);
+ testZlibCompression("hello, world!", CompressionScheme.BestSizeCompression);
+ testZlibCompression("hello, world!", CompressionScheme.BestSpeedCompression);
+ testZlibCompression("hello, world!", CompressionScheme.DefaultCompression);
const size_t kSize = 1024;
char BinaryData[kSize];
@@ -54,38 +66,27 @@
BinaryData[i] = i & 255;
StringRef BinaryDataStr(BinaryData, kSize);
- testZlibCompression(BinaryDataStr, zlib::NoCompression);
- testZlibCompression(BinaryDataStr, zlib::BestSizeCompression);
- testZlibCompression(BinaryDataStr, zlib::BestSpeedCompression);
- testZlibCompression(BinaryDataStr, zlib::DefaultCompression);
+ testZlibCompression(BinaryDataStr, CompressionScheme.BestSizeCompression);
+ testZlibCompression(BinaryDataStr, CompressionScheme.BestSpeedCompression);
+ testZlibCompression(BinaryDataStr, CompressionScheme.DefaultCompression);
}
#endif
#if LLVM_ENABLE_ZSTD
-static void testZstdCompression(StringRef Input, int Level) {
- SmallVector Compressed;
- SmallVector Uncompressed;
- zstd::compress(arrayRefFromStringRef(Input), Compressed, Level);
- // Check that uncompressed buffer is the same as original.
- Error E = zstd::uncompress(Compressed, Uncompressed, Input.size());
- consumeError(std::move(E));
-
- EXPECT_EQ(Input, toStringRef(Uncompressed));
- if (Input.size() > 0) {
-// Uncompression fails if expected length is too short.
-E = zstd::uncompress(Compressed, Uncompressed, Input.size() - 1);
-EXPECT_EQ("Destination buffer is too small", llvm::toString(std::move(E)));
- }
+static void
[PATCH] D130516: [Support] compression classes
ckissane created this revision.
ckissane added a reviewer: dblaikie.
Herald added subscribers: wenlei, usaxena95, kadircet, arphaman, hiraditya,
arichardson, emaste.
Herald added a reviewer: alexander-shaposhnikov.
Herald added a reviewer: rupprecht.
Herald added a reviewer: jhenderson.
Herald added a reviewer: MaskRay.
Herald added a project: All.
ckissane requested review of this revision.
Herald added subscribers: cfe-commits, llvm-commits, StephenFan.
Herald added projects: clang, LLVM, clang-tools-extra.
compression algorithm base class, two (or three, if it's useful to have a
default implementation for "None") derived classes, singleton instances of each
and a function that takes the algorithm type and returns the handler - which
can then be queried for "is available" and provides compress/decompress
actions. That'd mean having only one switch instead of three, but the three are
now so close together that it's not hugely painful to maintain (though having a
class hierarchy would ensure the APIs were actually identical, whereas
currently they're only identical by convention).
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D130516
Files:
clang-tools-extra/clangd/index/Serialization.cpp
clang-tools-extra/clangd/unittests/SerializationTests.cpp
clang/lib/Driver/ToolChains/Clang.cpp
clang/lib/Serialization/ASTReader.cpp
clang/lib/Serialization/ASTWriter.cpp
lld/ELF/Driver.cpp
lld/ELF/InputSection.cpp
llvm/include/llvm/Object/Decompressor.h
llvm/include/llvm/Support/Compression.h
llvm/lib/MC/ELFObjectWriter.cpp
llvm/lib/ObjCopy/ELF/ELFObject.cpp
llvm/lib/Object/Decompressor.cpp
llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
llvm/lib/ProfileData/InstrProf.cpp
llvm/lib/ProfileData/SampleProfReader.cpp
llvm/lib/ProfileData/SampleProfWriter.cpp
llvm/lib/Support/Compression.cpp
llvm/tools/llvm-mc/llvm-mc.cpp
llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
llvm/unittests/ProfileData/InstrProfTest.cpp
llvm/unittests/Support/CompressionTest.cpp
Index: llvm/unittests/Support/CompressionTest.cpp
===
--- llvm/unittests/Support/CompressionTest.cpp
+++ llvm/unittests/Support/CompressionTest.cpp
@@ -22,31 +22,43 @@
namespace {
-#if LLVM_ENABLE_ZLIB
-static void testZlibCompression(StringRef Input, int Level) {
+static void testCompressionAlgorithm(
+StringRef Input, int Level,
+compression::CompressionAlgorithm CompressionScheme,
+std::string ExpectedDestinationBufferTooSmallErrorMessage) {
SmallVector Compressed;
SmallVector Uncompressed;
- zlib::compress(arrayRefFromStringRef(Input), Compressed, Level);
+ CompressionScheme.compress(arrayRefFromStringRef(Input), Compressed, Level);
// Check that uncompressed buffer is the same as original.
- Error E = zlib::uncompress(Compressed, Uncompressed, Input.size());
+ Error E =
+ CompressionScheme.decompress(Compressed, Uncompressed, Input.size());
consumeError(std::move(E));
EXPECT_EQ(Input, toStringRef(Uncompressed));
if (Input.size() > 0) {
// Uncompression fails if expected length is too short.
-E = zlib::uncompress(Compressed, Uncompressed, Input.size() - 1);
-EXPECT_EQ("zlib error: Z_BUF_ERROR", llvm::toString(std::move(E)));
+E = CompressionScheme.decompress(Compressed, Uncompressed,
+ Input.size() - 1);
+EXPECT_EQ(ExpectedDestinationBufferTooSmallErrorMessage,
+ llvm::toString(std::move(E)));
}
}
+#if LLVM_ENABLE_ZLIB
+static void testZlibCompression(StringRef Input, int Level) {
+ testCompressionAlgorithm(Input, Level, ZlibCompressionAlgorithm(),
+ "zlib error: Z_BUF_ERROR");
+}
+
TEST(CompressionTest, Zlib) {
- testZlibCompression("", zlib::DefaultCompression);
+ compression::CompressionAlgorithm CompressionScheme =
+ compression::ZlibCompressionAlgorithm();
+ testZlibCompression("", CompressionScheme.DefaultCompression);
- testZlibCompression("hello, world!", zlib::NoCompression);
- testZlibCompression("hello, world!", zlib::BestSizeCompression);
- testZlibCompression("hello, world!", zlib::BestSpeedCompression);
- testZlibCompression("hello, world!", zlib::DefaultCompression);
+ testZlibCompression("hello, world!", CompressionScheme.BestSizeCompression);
+ testZlibCompression("hello, world!", CompressionScheme.BestSpeedCompression);
+ testZlibCompression("hello, world!", CompressionScheme.DefaultCompression);
const size_t kSize = 1024;
char BinaryData[kSize];
@@ -54,38 +66,27 @@
BinaryData[i] = i & 255;
StringRef BinaryDataStr(BinaryData, kSize);
- testZlibCompression(BinaryDataStr, zlib::NoCompression);
- testZlibCompression(BinaryDataStr, zlib::BestSizeCompression);
- testZlibCompression(BinaryDataStr, zlib::BestSpeedCompression);
- testZlibCompression(BinaryDataStr,
