Adding shrinking flag for cover and fastcover #1656

Tyler-Tran · 2019-06-18T23:39:24Z

Created a flag that allows the dictionary builder to select the smallest dictionary that is within a 1.01 regression of the largest dictionary.

lib/dictBuilder/fastcover.c

lib/dictBuilder/cover.c

lib/dictBuilder/zdict.h

programs/zstdcli.c

lib/dictBuilder/cover.c

terrelln · 2019-06-21T21:12:40Z

I haven't reviewed yet, but can you please add a test case in tests/playTests.sh, where the other dictionary builder tests are?

terrelln · 2019-06-21T21:13:35Z

Also, lets add a test case in tests/fuzzer.c where the other dict builder tests are

terrelln

Overall looking good, just some nits.

lib/dictBuilder/cover.c

terrelln

Mostly nits, but 2 changes:

I think there is a bug when no smaller dictionary is found.
Please switch to --train-cover=shrink=1 instead of --shrink-dict. See how split is implemented.

lib/dictBuilder/cover.c

programs/zstdcli.c

tests/fuzzer.c

tests/playTests.sh

terrelln

Please update programs/zstd.1.md, which is the man page for zstd, to say what shrink does. Search for --train-cover and --train-fastcover to see where to update.

terrelln · 2019-06-26T01:04:47Z

lib/dictBuilder/cover.c

+
+  BYTE *const largestDictbuffer = (BYTE * const)malloc(dictContentSize);
+  BYTE *const candidateDictBuffer = (BYTE * const)malloc(dictContentSize);
+  double regressionTolerance = params.shrinkDict + 1.00;


I liked taking the parameter as an unsigned, but we should distinguish:

Auto shrinking enabled vs disabled. If you need fast dictionary training, you don't want auto shrinking.
It can be disabled by default.

The regression we allow. A regression of 0 could make sense if you want to take a smaller dictionary only if it is better.

I think we could take two flags shrinkDict and shrinkDictMaxRegression, both unsigned. It was a mistake to take split as a double, so I don't want to repeat it here.

On the library side, we can default to disabled, and the regression defaults to 0.

On the CLI, we can default to disabled, but we could say if someone sets --train-fastcover=shrink we enable with default to a regression of 1, but --train-fastcover=shrink=0 or --train-fastcover=shrink=2 should both still work.

terrelln · 2019-06-26T01:15:47Z

lib/dictBuilder/cover.c

-                                                       dict, dictBufferCapacity);
+    dictBufferCapacity = selection.dictSize;
+    totalCompressedSize = selection.totalCompressedSize;
+    memcpy(dict, selection.dictContent, dictBufferCapacity);


We no longer need dict. If we move the declaration of selection to the top scope, we can refactor COVER_best_finish() to take a COVER_dictSelection instead of the three parameters (dict, totalCompressedSize, dictBufferCapacity).

COVER_dictSelection_t selection = COVER_dictSelectionError(ERROR(GENERIC)); /* ... */ { const size_t tail = COVER_buildDictionary(/* ... */); selection = COVER_selectDict(/* ... */); } _cleanup: COVER_best_finish(data->best, parameters, selection); /* Copies the dict out of selection */ COVER_dictSelectionFree(selection); ``

terrelln · 2019-06-26T01:19:29Z

programs/zstdcli.c

@@ -311,10 +311,15 @@ static unsigned parseCoverParameters(const char* stringPtr, ZDICT_cover_params_t
          params->splitPoint = (double)splitPercentage / 100.0;
          if (stringPtr[0]==',') { stringPtr++; continue; } else break;
        }
+        if (longCommandWArg(&stringPtr, "shrink=")) {


Lets allow shrink to set a default regression amount of 1.

if (longCommandWArg(&stringPtr, "shrink")) { unsigned regression = kDefaultRegression; if (stringPtr[0] == '=') { stringPtr++; regression = readU32FromChar(&stringPtr); } /* ... */

terrelln

Looks like the CI is broken.

terrelln · 2019-06-26T22:23:21Z

lib/dictBuilder/zdict.h

@@ -94,7 +94,8 @@ typedef struct {
    unsigned steps;              /* Number of steps : Only used for optimization : 0 means default (40) : Higher means more parameters checked */
    unsigned nbThreads;          /* Number of threads : constraint: 0 < nbThreads : 1 means single-threaded : Only used for optimization : Ignored if ZSTD_MULTITHREAD is not defined */
    double splitPoint;           /* Percentage of samples used for training: Only used for optimization : the first nbSamples * splitPoint samples will be used to training, the last nbSamples * (1 - splitPoint) samples will be used for testing, 0 means default (1.0), 1.0 when all samples are used for both training and testing */
-    double shrinkDict;           /* Shrink dictionaries to select the smallest dictionary within the % specified by the auto-shrink-dict flag: 0 disables shrinking  */
+    unsigned shrinkDict;         /* Train dictionaries to shrink in size starting from the minimum size and selects the smallest dictionary within the max regression. 0 means no shrinking and 1 means shrinking  */
+    unsigned shrinkDictMaxRegression; /* Sets the value that determines how much worse a dictionary can be than the dictionary of max dictionary size */


Can you describe what this value means? E.g. A smaller dict is allowed to be shrinkDictMaxRegression worse than the full sized dictionary.

terrelln · 2019-06-26T22:23:50Z

programs/zstdcli.c

@@ -179,8 +179,8 @@ static int usage_advanced(const char* programName)
    DISPLAY( "\n");
    DISPLAY( "Dictionary builder : \n");
    DISPLAY( "--train ## : create a dictionary from a training set of files \n");
-    DISPLAY( "--train-cover[=k=#,d=#,steps=#,split=#,shrink=#] : use the cover algorithm with optional args\n");
-    DISPLAY( "--train-fastcover[=k=#,d=#,f=#,steps=#,split=#,accel=#,shrink=#] : use the fast cover algorithm with optional args\n");
+    DISPLAY( "--train-cover[=k=#,d=#,steps=#,split=#,shrink] : use the cover algorithm with optional args\n");


shrink[=#] brackets means optional arguments

terrelln · 2019-06-26T22:24:39Z

programs/zstdcli.c

@@ -377,7 +394,8 @@ static ZDICT_cover_params_t defaultCoverParams(void)
    params.d = 8;
    params.steps = 4;
    params.splitPoint = 1.0;
-    params.shrinkDict = 0.0;
+    params.shrinkDict = 0;
+    params.shrinkDictMaxRegression = 1;


kDefaultRegression

terrelln · 2019-06-26T22:24:42Z

programs/zstdcli.c

@@ -390,7 +408,8 @@ static ZDICT_fastCover_params_t defaultFastCoverParams(void)
    params.steps = 4;
    params.splitPoint = 0.75; /* different from default splitPoint of cover */
    params.accel = DEFAULT_ACCEL;
-    params.shrinkDict = 0.0;
+    params.shrinkDict = 0;
+    params.shrinkDictMaxRegression = 1;


kDefaultRegression

terrelln · 2019-06-26T22:26:05Z

lib/dictBuilder/cover.c

  free(data);
  COVER_map_destroy(&activeDmers);
-  if (dict) {
-    free(dict);


It looks like you're leaking dict.

terrelln · 2019-06-26T22:26:52Z

lib/dictBuilder/fastcover.c

  free(data);
  free(segmentFreqs);
-  free(dict);


Looks like you're leaking dict.

terrelln

A one line fix and this looks good to go!

terrelln · 2019-06-27T19:42:03Z

lib/dictBuilder/cover.c

@@ -929,6 +934,111 @@ void COVER_best_finish(COVER_best_t *best, size_t compressedSize,
  }
 }

+COVER_dictSelection_t COVER_dictSelectionError(size_t error) {
+    COVER_dictSelection_t selection = { NULL, error, error };


You need to set dictSize == 0 here.

In COVER_best_finish() when totalCompressedSize is an error, but still < -1 we will allocate space for the dictionary. We will try to allocate ~2^64 bytes, which will probably fail, but still we don't want to do that.

The dictionary size should always be the right number here, which is 0.

Tyler-Tran added 20 commits May 22, 2019 09:39

Changed ERROR(GENERIC) excluding inits

91fd73d

editing git ignore

ac0441a

Edited init functions to size_t returns

68dc307

moved declarations earlier

d006650

resolved issues with changes to init functions

3e4fa2e

fixed style and an error check

f684bd3

attempting to add tests that might trigger changes

f937124

added && die to cases expecting to fail

a9fe0ee

resolved no die on expected failed command

16a4d69

fixed accel to be incorrect value

da49a01

Merge https://github.com/facebook/zstd into dev

dd35330

Adding an automated shrinking option

3893b55

Fixing build

a8e3751

finalizing fixes

1c1d329

fix?

07f5bee

Removing added comment in cover.h

81894bb

Styling fixes

7101abb

Merging with fb dev

5384220

Merge https://github.com/facebook/zstd into dev

e0a9ad2

removing megic number for default regression

e5cb67f

facebook-github-bot added the CLA Signed label Jun 18, 2019

Cyan4973 requested a review from terrelln June 18, 2019 23:47

terrelln requested changes Jun 19, 2019

View reviewed changes

Tyler-Tran added 7 commits June 19, 2019 14:15

Requested revisions

b333938

fixing support for fast cover

ebf8281

fixing casting errors

a1cb744

parenthesis fix

5a220ea

fixing some build nits

3443e6a

resolving travis ci syntax

9bc8d1b

might resolve all compilation issues

295010e

Tyler-Tran added 7 commits June 20, 2019 10:03

removed unused variable

b40ada1

remodeling the selectDict function

7dc2c4b

fixing bad memory access

1668ba7

fixing error checks

0a6eba1

fixed erroring check in selectDict

343af4e

fixing mixed declarations

b4b6046

modify mixed declaration

29ede28

terrelln requested changes Jun 21, 2019

View reviewed changes

fixing nits and adding test cases

d556f63

terrelln reviewed Jun 24, 2019

View reviewed changes

Tyler-Tran added 3 commits June 25, 2019 11:22

Adding requested changes + fixed bug for error checking

7b3aa91

switched double comparison from != to <

722dbba

fixed declaration typing

4e06997

terrelln reviewed Jun 26, 2019

View reviewed changes

refactoring COVER_best_finish() and changing shrinkDict

84397b7

terrelln reviewed Jun 26, 2019

View reviewed changes

Tyler-Tran added 3 commits June 26, 2019 16:37

removing the const's

593a8e0

modifying ZDICT_optimizeTrainFromBuffer_cover functions

e59ad22

fixing potential bad memcpy

7f6ac88

terrelln reviewed Jun 27, 2019

View reviewed changes

fixing the error function for dict size

9827132

terrelln approved these changes Jun 27, 2019

View reviewed changes

terrelln merged commit c55d2e7 into facebook:dev Jun 27, 2019

felixhandte mentioned this pull request Jul 19, 2019

Merge v1.4.1 to Master #1691

Merged

Tyler-Tran added a commit to Tyler-Tran/zstd that referenced this pull request Jul 22, 2019

Adding documentation for shrink flag PR facebook#1656

02da449

Tyler-Tran mentioned this pull request Jul 24, 2019

Adding documentation for --shrink flag #1697

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding shrinking flag for cover and fastcover #1656

Adding shrinking flag for cover and fastcover #1656

Tyler-Tran commented Jun 18, 2019

terrelln commented Jun 21, 2019

terrelln commented Jun 21, 2019

terrelln left a comment

terrelln left a comment

terrelln left a comment

terrelln Jun 26, 2019

terrelln Jun 26, 2019

terrelln Jun 26, 2019

terrelln left a comment

terrelln Jun 26, 2019

terrelln Jun 26, 2019

terrelln Jun 26, 2019

terrelln Jun 26, 2019

terrelln Jun 26, 2019

terrelln Jun 26, 2019

terrelln left a comment

terrelln Jun 27, 2019

Adding shrinking flag for cover and fastcover #1656

Adding shrinking flag for cover and fastcover #1656

Conversation

Tyler-Tran commented Jun 18, 2019

terrelln commented Jun 21, 2019

terrelln commented Jun 21, 2019

terrelln left a comment

Choose a reason for hiding this comment

terrelln left a comment

Choose a reason for hiding this comment

terrelln 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

terrelln 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

terrelln left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment