HPCC-29917 Refactor compressToBuffer to support different compression methods #18009

richardkchapman · 2023-11-08T15:53:52Z

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

ghalliday

One comment, and one issue to investigate

ghalliday · 2023-11-20T10:17:32Z

system/jlib/jlzw.cpp

+            if ((method & ~COMPRESS_METHOD_AES) < COMPRESS_METHOD_LAST)
+            {
+                if (method & COMPRESS_METHOD_AES)
+                    AESbyMethod[method & ~COMPRESS_METHOD_AES] = handler;


= nullptr
and below.

ghalliday · 2023-11-20T10:25:07Z

system/jlib/jlzw.cpp

        void *newData = out.reserve(newSize);
-        compressor->open(newData, newSize);
-        if (compressor->write(src, len)==len)
+        try


Future: Could use compressToBlock() if supported by the compressor. There is also a few bytes wasted, which would be more significant for small sizes, but that requires substantial refactoring.

I have created a separate jira for this HPCC-30928 (and a branch gch/issue30928). It has two effects
i) it compresses better due to the daft way the current lz4 compress works.
ii) it is faster.
Before:

12:24:58.258158 419929 compressToBuffer 5 size 0 did not compress 12:24:58.258387 419929 compressToBuffer 5 size 102 compressed to 66 in 1985ns 12:24:58.258803 419929 compressToBuffer 5 size 1000 compressed to 53 in 1503ns

After:

12:25:21.154246 420137 compressToBuffer 5 size 0 did not compress 12:25:21.154313 420137 compressToBuffer 5 size 34 compressed to 32 in 2434ns 12:25:21.154783 420137 compressToBuffer 5 size 1000 compressed to 35 in 1082ns

We could remove 4 bytes from the size, but that would be trickier (the format wouldn't be compatible).

With that change, switching to a less repetitive text (Gen 1 from 0drvb10), LZW is better at compressing, but significantly slower:

12:37:32.829295 422255 compressToBuffer 2 size 512 compressed to 357 in 5490ns 12:37:32.837024 422255 compressToBuffer 2 size 1000 compressed to 627 in 9808ns 12:37:32.838347 422255 compressToBuffer 5 size 512 compressed to 411 in 1893ns 12:37:32.841931 422255 compressToBuffer 5 size 1000 compressed to 717 in 2885ns

ghalliday · 2023-11-20T10:38:14Z

testing/unittests/unittests.cpp

+        testSome(0);
+        testSome(1);
+        testSome(16);
+        testSome(32);


Adding testSome(48) or 64 causes a realloc error.

jakesmith

@richardkchapman - looks good, some trivial comments.

jakesmith · 2023-11-22T10:30:17Z

system/jlib/jfcmp.hpp

-        if (max<1024)
-            throw MakeStringException(-1,"CFcmpCompressor::open - block size (%d) not large enough", max);
+//        if (max<1024)
+//            throw MakeStringException(-1,"CFcmpCompressor::open - block size (%d) not large enough", max);


would be good to delete if no longer applicable (and similarly on line 106-7)

jakesmith · 2023-11-22T10:33:22Z

system/jlib/jlzw.cpp

+        if (!handler)
+        {
+            VStringBuffer s("Unknown compression method %x requested in compressToBuffer", (byte) method);
+            throw makeStringException(0, s.str());


trivial: use makeStringExceptionV?

jakesmith · 2023-11-22T10:35:21Z

system/jlib/jlzw.cpp

        }
+        catch (IException *E)
+        {
+            E->Release();


should there a debug log of the exception here?

I don't think so - it will only happen for aes encryption of small buffers.

jakesmith · 2023-11-22T10:46:19Z

system/jlib/jlzw.cpp

+                compressor->close();
+                size32_t compressedLen = compressor->buflen();
+                out.setWritePos(originalLength + sizeof(byte));
+                out.append(compressedLen);


trivial: could use a DelayedMarker<size32_t> instead of line 777, and a marker.write(compressedLen) here

ghalliday · 2023-11-28T11:49:46Z

testing/unittests/unittests.cpp

+            if (!prevResult)
+                DBGLOG("compressToBuffer %x size %u compressed to %u", (byte) method, len, compressed.length());
+            ret = true;
+        }
        CPPUNIT_ASSERT(compressed.length() <= len+5);
        MemoryBuffer out;
        decompressToBuffer(out, compressed, options);
        CPPUNIT_ASSERT(out.length() == len);
        CPPUNIT_ASSERT(memcmp(out.bytes(), in, len) == 0);


only check if len != 0, otherwise you fail a sanity check in debug

ghalliday

@richardkchapman one problem with sanity check, but if that is fixed it looks ok to merge.
Please can you target it to 9.4.x. (I agree it is too painful to merge further back.)

ghalliday · 2023-11-28T12:28:19Z

system/jlib/jlz4.cpp

@@ -73,7 +72,7 @@ class CLZ4Compressor final : public CFcmpCompressor
        if (toflush == 0)
            return;

-        if (toflush < 256)
+        if (false && toflush < 256)


Should clean up and delete this code.

ghalliday · 2023-11-28T12:41:17Z

system/jlib/jlzw.cpp

        void *newData = out.reserve(newSize);
-        compressor->open(newData, newSize);
-        if (compressor->write(src, len)==len)
+        try


I have created a separate jira for this HPCC-30928 (and a branch gch/issue30928). It has two effects
i) it compresses better due to the daft way the current lz4 compress works.
ii) it is faster.
Before:

12:24:58.258158 419929 compressToBuffer 5 size 0 did not compress 12:24:58.258387 419929 compressToBuffer 5 size 102 compressed to 66 in 1985ns 12:24:58.258803 419929 compressToBuffer 5 size 1000 compressed to 53 in 1503ns

After:

12:25:21.154246 420137 compressToBuffer 5 size 0 did not compress 12:25:21.154313 420137 compressToBuffer 5 size 34 compressed to 32 in 2434ns 12:25:21.154783 420137 compressToBuffer 5 size 1000 compressed to 35 in 1082ns

We could remove 4 bytes from the size, but that would be trickier (the format wouldn't be compatible).

With that change, switching to a less repetitive text (Gen 1 from 0drvb10), LZW is better at compressing, but significantly slower:

12:37:32.829295 422255 compressToBuffer 2 size 512 compressed to 357 in 5490ns 12:37:32.837024 422255 compressToBuffer 2 size 1000 compressed to 627 in 9808ns 12:37:32.838347 422255 compressToBuffer 5 size 512 compressed to 411 in 1893ns 12:37:32.841931 422255 compressToBuffer 5 size 1000 compressed to 717 in 2885ns

ghalliday · 2023-11-28T12:42:14Z

system/jlib/jlzw.cpp

        }
+        catch (IException *E)
+        {
+            E->Release();


I don't think so - it will only happen for aes encryption of small buffers.

richardkchapman · 2023-12-04T11:11:48Z

@ghalliday I have fixed the sanitize check issue with len=0, removed disabled code, retargeted to 9.4.x and squashed. Please re-review and merge if accepted.

ghalliday

Unfortunately this has created a regression. If you create workunits (and deploy to roxie) with the old code, and then read with the new code the system, crashes.

It is beause
void decompressToBuffer(MemoryBuffer & out, const void * src)

does not have a leading version - it is processed in decompressResource. I think the correct fix is to pass version (and len) from decompress resource into a shared function.

ghalliday · 2023-12-04T17:41:56Z

Tested with that change and it fixed the regression. The compressed file size varies slightly (due to the removal of the 256 limit). For the performance suite (admittedly not representative) the timings did not vary, sizes were as follows:

Before:
-rw-r--r-- 1 gavin gavin  4120023230 Dec  4 12:13 padded_compressed._1_of_1
-rw-r--r-- 1 gavin gavin  3650671057 Dec  4 12:13 simple_compressed._1_of_1
After:
-rw-r--r-- 1 gavin gavin  4119595073 Dec  4 17:02 padded_compressed._1_of_1
-rw-r--r-- 1 gavin gavin  3650689570 Dec  4 17:02 simple_compressed._1_of_1

The first is slightly smaller. The second is very slightly bigger - which is interesting but insignificant. I would expect the files to be slightly smaller, but occasionally bigger because it will change the boundaries of the blocks of data being compressed.

ghalliday

@richardkchapman please squash.

… methods Also add some tests, and fix some issues that the tests showed up with small input sizes. Ensure back compatibility for LZW LE compression Signed-off-by: Richard Chapman <rchapman@hpccsystems.com>

richardkchapman · 2023-12-13T10:38:00Z

@ghalliday Squashed

richardkchapman force-pushed the compressToBuffer branch 2 times, most recently from 8949410 to c25ff6e Compare November 15, 2023 15:04

richardkchapman changed the title ~~Compress to buffer~~ HPCC-29917 Refactor compressToBuffer to support different compression methods Nov 15, 2023

richardkchapman requested review from ghalliday and jakesmith November 15, 2023 15:05

richardkchapman marked this pull request as ready for review November 15, 2023 15:06

ghalliday reviewed Nov 20, 2023

View reviewed changes

jakesmith reviewed Nov 22, 2023

View reviewed changes

jakesmith approved these changes Nov 22, 2023

View reviewed changes

richardkchapman force-pushed the compressToBuffer branch from de1f5ba to 35bab01 Compare November 22, 2023 12:34

ghalliday reviewed Nov 28, 2023

View reviewed changes

richardkchapman force-pushed the compressToBuffer branch from 35bab01 to 053ac0b Compare December 4, 2023 11:09

richardkchapman changed the base branch from master to candidate-9.4.x December 4, 2023 11:09

richardkchapman force-pushed the compressToBuffer branch from 053ac0b to 1a2e915 Compare December 4, 2023 11:10

richardkchapman requested a review from ghalliday December 4, 2023 11:10

ghalliday requested changes Dec 4, 2023

View reviewed changes

ghalliday approved these changes Dec 4, 2023

View reviewed changes

HPCC-29917 Refactor compressToBuffer to support different compression…

f59328b

… methods Also add some tests, and fix some issues that the tests showed up with small input sizes. Ensure back compatibility for LZW LE compression Signed-off-by: Richard Chapman <rchapman@hpccsystems.com>

richardkchapman force-pushed the compressToBuffer branch from 2e6ca90 to f59328b Compare December 13, 2023 10:37

ghalliday merged commit 08425ea into hpcc-systems:candidate-9.4.x Dec 15, 2023
47 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-29917 Refactor compressToBuffer to support different compression methods #18009

HPCC-29917 Refactor compressToBuffer to support different compression methods #18009

richardkchapman commented Nov 8, 2023 •

edited

Loading

ghalliday left a comment

ghalliday Nov 20, 2023

ghalliday Nov 20, 2023

ghalliday Nov 28, 2023 •

edited

Loading

ghalliday Nov 20, 2023

jakesmith left a comment

jakesmith Nov 22, 2023

jakesmith Nov 22, 2023

jakesmith Nov 22, 2023

ghalliday Nov 28, 2023

jakesmith Nov 22, 2023

ghalliday Nov 28, 2023

ghalliday left a comment

ghalliday Nov 28, 2023

ghalliday Nov 28, 2023 •

edited

Loading

ghalliday Nov 28, 2023

richardkchapman commented Dec 4, 2023

ghalliday left a comment

ghalliday commented Dec 4, 2023

ghalliday left a comment

richardkchapman commented Dec 13, 2023

HPCC-29917 Refactor compressToBuffer to support different compression methods #18009

HPCC-29917 Refactor compressToBuffer to support different compression methods #18009

Conversation

richardkchapman commented Nov 8, 2023 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardkchapman commented Dec 4, 2023

ghalliday left a comment

Choose a reason for hiding this comment

ghalliday commented Dec 4, 2023

ghalliday left a comment

Choose a reason for hiding this comment

richardkchapman commented Dec 13, 2023

richardkchapman commented Nov 8, 2023 •

edited

Loading

ghalliday Nov 28, 2023 •

edited

Loading

ghalliday Nov 28, 2023 •

edited

Loading