Rewrite check *bufStart condition #3304

GermanAizek · 2022-10-30T21:11:40Z

Rewriting logic bufStart pointer validation condition and micro-optimization with strlen() function. For DYNAMIC_BMI2 macro, it was meant to use a function with the *_bmi2 postfix. After review PR code, feedback about it.

embg

Thanks for the PR! I like your change related to *bufStart == NULL, but for the other changes I prefer the original code. In many cases the compiler will take care of the optimizations you're doing manually.

If you revert those other changes, happy to merge!

embg · 2022-10-31T15:42:12Z

lib/common/entropy_common.c

@@ -200,7 +200,7 @@ BMI2_TARGET_ATTRIBUTE static size_t FSE_readNCount_body_bmi2(
        short* normalizedCounter, unsigned* maxSVPtr, unsigned* tableLogPtr,
        const void* headerBuffer, size_t hbSize)
 {
-    return FSE_readNCount_body(normalizedCounter, maxSVPtr, tableLogPtr, headerBuffer, hbSize);
+    return FSE_readNCount_body_bmi2(normalizedCounter, maxSVPtr, tableLogPtr, headerBuffer, hbSize);


The original code here is actually correct :) BMI2_TARGET_ATTRIBUTE tells the compiler to use BMI2 instructions when compiling this function. But the source code of the function doesn't change.

The reason you're seeing failures in CI is that you're defining FSE_readNCount_body_bmi2() in terms of itself, i.e. you have an infinite recursion.

embg · 2022-10-31T15:42:51Z

programs/util.c

@@ -386,7 +386,7 @@ static size_t readLineFromFile(char* buf, size_t len, FILE* file)
    assert(!feof(file));
    if ( fgets(buf, (int) len, file) == NULL ) return 0;
    {   size_t linelen = strlen(buf);
-        if (strlen(buf)==0) return 0;
+        if (buf[0]=='\0') return 0;


I think we can rely on the compiler to make this optimization, so I prefer the original code here since it's more readable.

embg · 2022-10-31T15:45:53Z

programs/util.c

-                *bufEnd = *bufStart + newListSize;
-                if (*bufStart == NULL) { free(path); closedir(dir); return 0; }
+                if (*bufStart != NULL) *bufEnd = *bufStart + newListSize;
+                else { free(path); closedir(dir); return 0; }


I think this is a useful change since performing arithmetic on a NULL pointer is UB. Can you please add curly braces around *bufEnd = *bufStart + newListSize;?

embg · 2022-10-31T15:49:27Z

programs/zstdcli.c

-        (exeName[strlen(test)] == '\0' || exeName[strlen(test)] == '.');
+    size_t len = strlen(test);
+    return !strncmp(exeName, test, len) &&
+        (exeName[len] == '\0' || exeName[len] == '.');


The compiler will take care of this optimization, so I don't think we need to change this code.

embg · 2022-10-31T15:49:46Z

programs/zstdcli.c

@@ -1037,7 +1038,7 @@ int main(int argCount, const char* argv[])
                if (longCommandWArg(&argument, "--size-hint")) { NEXT_TSIZE(srcSizeHint); continue; }
                if (longCommandWArg(&argument, "--output-dir-flat")) {
                    NEXT_FIELD(outDirName);
-                    if (strlen(outDirName) == 0) {
+                    if (outDirName[0] == '\0') {


Same as above :)

embg · 2022-10-31T15:49:52Z

programs/zstdcli.c

@@ -1053,7 +1054,7 @@ int main(int argCount, const char* argv[])
 #ifdef UTIL_HAS_MIRRORFILELIST
                if (longCommandWArg(&argument, "--output-dir-mirror")) {
                    NEXT_FIELD(outMirroredDirName);
-                    if (strlen(outMirroredDirName) == 0) {
+                    if (outMirroredDirName[0] == '\0') {


Same as above :)

embg · 2022-10-31T15:55:14Z

Here's a godbolt link showing that gcc won't actually call strlen for strlen(str) > 0: https://godbolt.org/z/zerrbcorr

GermanAizek · 2022-10-31T16:02:25Z

Here's a godbolt link showing that gcc won't actually call strlen for strlen(str) > 0: https://godbolt.org/z/zerrbcorr

I followed link and made sure that there was no optimization, and readability code suffered. I will revert changes and leave only with NULL checking.

facebook-github-bot · 2022-10-31T18:23:36Z

Hi @GermanAizek!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

GermanAizek · 2022-10-31T18:24:58Z

@embg, check this commit 1b7f3c4

embg · 2022-10-31T18:52:27Z

Looks good, you need to fix two things:

Sign the CLA: https://code.facebook.com/cla/individual
Add curly braces around both branches of the if statement, like this:

if (*bufStart != NULL) {
    *bufEnd = *bufStart + newListSize;
} else {
    free(path); closedir(dir); return 0;
}

facebook-github-bot · 2022-10-31T23:31:48Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

GermanAizek · 2022-10-31T23:33:48Z

@embg, everything is ready for PR merge.

embg · 2022-11-01T14:41:01Z

Thanks for the contribution! Will merge after CI passes.

embg requested changes Oct 31, 2022

View reviewed changes

GermanAizek force-pushed the dev branch from ee8c557 to 1b7f3c4 Compare October 31, 2022 18:23

Rewrite checking condition bufStart pointer NULL

5334339

GermanAizek force-pushed the dev branch from 1b7f3c4 to 5334339 Compare October 31, 2022 23:31

facebook-github-bot added the CLA Signed label Oct 31, 2022

GermanAizek changed the title ~~Rewrite check *bufStart, optimize strlen() and fixed FSE_readNCount_body_bmi2() for DYNAMIC_BMI2~~ Rewrite check *bufStart condition Oct 31, 2022

embg approved these changes Nov 1, 2022

View reviewed changes

embg merged commit bb23f7b into facebook:dev Nov 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite check *bufStart condition #3304

Rewrite check *bufStart condition #3304

GermanAizek commented Oct 30, 2022

embg left a comment

embg Oct 31, 2022

embg Oct 31, 2022

embg Oct 31, 2022

embg Oct 31, 2022

embg Oct 31, 2022

embg Oct 31, 2022

embg commented Oct 31, 2022

GermanAizek commented Oct 31, 2022

facebook-github-bot commented Oct 31, 2022

GermanAizek commented Oct 31, 2022

embg commented Oct 31, 2022

facebook-github-bot commented Oct 31, 2022

GermanAizek commented Oct 31, 2022

embg commented Nov 1, 2022

Rewrite check *bufStart condition #3304

Rewrite check *bufStart condition #3304

Conversation

GermanAizek commented Oct 30, 2022

embg left a comment

Choose a reason for hiding this comment

embg Oct 31, 2022

Choose a reason for hiding this comment

embg Oct 31, 2022

Choose a reason for hiding this comment

embg Oct 31, 2022

Choose a reason for hiding this comment

embg Oct 31, 2022

Choose a reason for hiding this comment

embg Oct 31, 2022

Choose a reason for hiding this comment

embg Oct 31, 2022

Choose a reason for hiding this comment

embg commented Oct 31, 2022

GermanAizek commented Oct 31, 2022

facebook-github-bot commented Oct 31, 2022

Action Required

Process

GermanAizek commented Oct 31, 2022

embg commented Oct 31, 2022

facebook-github-bot commented Oct 31, 2022

GermanAizek commented Oct 31, 2022

embg commented Nov 1, 2022