Fix Issue 18223: use memset in uninitializedFillDefault(T[]) by n8sh · Pull Request #6024 · dlang/phobos

n8sh · 2018-01-11T06:38:34Z

Current function in std.experimental.allocator.package:

private T[] uninitializedFillDefault(T)(T[] array) nothrow
{
    T t = T.init;
    fillWithMemcpy(array, t);
    return array;
}

When we can statically determine that the representation of T.init consists of nothing but zeroes we can instead use memset. char and wchar can also be special-cased.

dlang-bot · 2018-01-11T06:38:37Z

Thanks for your pull request and interest in making D better, @n8sh! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	18223	enhancement	std.experimental.allocator uninitializedFillDefault could use memset

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#6024"

n8sh · 2018-01-11T06:40:16Z

std/experimental/allocator/common.d

+zero bits? Padding between a struct's fields is not considered.
+/
+private template isAllZeroBits(T, T value)
+{


Is there a more elegant way than this to check if T.init is entirely zero bits? typeid(T).initializer() is null isn't available.

n8sh · 2018-01-11T06:41:22Z

std/experimental/allocator/common.d

+                foreach (i; 1 .. N)
+                    buffer ~= " && isAllZeroBits!(E,value["~to!string(i)~"])";
+                return buffer;
+            }());


Is there a more elegant way to check all the elements of a static array?

n8sh · 2018-01-11T06:41:40Z

std/experimental/allocator/common.d

+                        ~"), value."~fieldName~")";
+            }
+            return buffer;
+        }());


Is there a more elegant way to check all the fields of a struct?

n8sh · 2018-01-11T06:46:52Z

std/experimental/allocator/common.d

+                    buffer ~= " && isAllZeroBits!(E,value["~to!string(i)~"])";
+                return buffer;
+            }());
+        }


Is there a more elegant way to check all the elements of a static array?

Your version in "elegant":

enum isAllZeroBits = () { int b; static foreach (e; value) b += !isAllZeroBits!(typeof(e), e); return b == 0; }();

And I think we can do even better here:

enum isAllZeroBits = isAllZeroBits!(typeof(value[0]), value[0]);

Static arrays need to have the same type, don't they?

That works only when the static array is the outermost type and not a field of another struct . That optimization could go in isInitAllZeroBits though.

DmitryOlshansky · 2018-01-11T10:39:33Z

std/experimental/allocator/common.d

+            enum isAllZeroBits = mixin(()
+            {
+                enum N = typeof(value).length;
+                char[] buffer = new char[32 * value.length]; // Pre-size for value.length <= 999.


This doesn't reserve the size. On append the slice is not the tail so it wil reallocate.

Thanks for the correction! reserve isn't available at compile time so I'll just remove this.

n8sh · 2018-01-11T23:48:38Z

Is there an escape hatch to bypass read-access protection during CTFE? Being unable to inspect private members of structs greatly reduces the applicability. Alternatively is there a better way to check at compile time if a struct's representation is entirely zero?

EDIT: Found the way using tupleof.

wilzbach · 2018-01-12T11:34:00Z

std/experimental/allocator/package.d

+        import core.stdc.string : memset;
+        if (array !is null)
+            memset(array.ptr, cast(ubyte) T.init, T.sizeof * array.length);
+        return array;


This isn't covered.

Added a unittest.

wilzbach · 2018-01-12T11:36:03Z

std/experimental/allocator/common.d

+        }());
+    }
+    else
+        enum isAllZeroBits = false;


The style guide demands that once braces are used for an if statement that they are used for all cases.

wilzbach · 2018-01-12T11:44:56Z

std/experimental/allocator/common.d

+                char[] buffer;
+                buffer ~= "isAllZeroBits!(E,value[0])";
+                import std.conv : to;
+                foreach (i; 1 .. N)


could be static foreach

Because it could have been either I avoided it due to seeing this advice on the forums:

When you can use the old style do so. Since it puts less stress on the compiler in the general case.

That's bad advice. There isn't any measurable performance penalty - I tried a large-scale test: #5989

wilzbach · 2018-01-12T11:45:33Z

std/experimental/allocator/common.d

+            enum isAllZeroBits = mixin(()
+            {
+                enum N = typeof(value).length;
+                char[] buffer;


could be string - you don't modify individual chars.

wilzbach · 2018-01-12T11:52:57Z

std/experimental/allocator/common.d

+                    buffer ~= " && isAllZeroBits!(E,value["~to!string(i)~"])";
+                return buffer;
+            }());
+        }


Your version in "elegant":

enum isAllZeroBits = () { int b; static foreach (e; value) b += !isAllZeroBits!(typeof(e), e); return b == 0; }();

wilzbach · 2018-01-12T12:06:09Z

std/experimental/allocator/common.d

+            if (buffer.length == 0)
+                buffer ~= "true";
+            return buffer;
+        }());


How about:

static if (value.tupleof.length == 0) { enum isAllZeroBits = true; } else { enum isAllZeroBits = () { int b; static foreach (e; value.tupleof) b += !isAllZeroBits!(typeof(e), e); return b == 0; }(); }

wilzbach · 2018-01-12T12:09:11Z

std/experimental/allocator/common.d

+                    buffer ~= " && isAllZeroBits!(E,value["~to!string(i)~"])";
+                return buffer;
+            }());
+        }


And I think we can do even better here:

enum isAllZeroBits = isAllZeroBits!(typeof(value[0]), value[0]);

Static arrays need to have the same type, don't they?

wilzbach

const(char)[] (aka string) is a typical use case, so I would add tests for arrays with const elements.

wilzbach · 2018-01-14T03:46:55Z

std/experimental/allocator/package.d

+            memset(array.ptr, 0, T.sizeof * array.length);
+        return array;
+    }
+    else static if (is(T : char) || (is(T : wchar) && T.init == 0xffff))


What's the reason for excluding dchar here?

Shouldn't the type classifiers be removed? E.g.

[Unqual]((https://dlang.org/phobos/std_traits.html#Unqual)

isSomeChar - doesn't allow alias this

Also the T.init == 0xffff check is to support user-extended versions of (char) that don't overwrite the init value, right? This should probably get a test too.

dchar.sizeof == 4 && dchar.init == 0x0000_ffff. Can't use memset.

static assert(is(immutable(char) : char)) compiles so I don't know that Unqual is necessary. I hadn't given much though to alias this. What do you suggest?

Since char.sizeof == 1 memset will work regardless of what char.init is. Since wchar.sizeof == 2 memset will work if the high 8 bits are the same as the low 8 bits; the main purpose of the T.init == 0xffff check was reminding the casual reader that is is true for wchar. Maybe0 == (T.init & 0xff) ^ (T.init >>> 8) would be better?

wilzbach · 2018-01-14T03:57:17Z

std/experimental/allocator/common.d

+    // Has non-zero private fields.
+    import std.random : Mt19937;
+    static assert(!isInitAllZeroBits!Mt19937);
+}


It might be worthwhile to check whether this works with const.
ConstOf might be helpful.

wilzbach · 2018-01-14T04:00:54Z

std/experimental/allocator/package.d

+    static assert(isInitAllZeroBits!P);
+    P[] a = [P(10, 11), P(20, 21), P(40, 41)];
+    uninitializedFillDefault(a);
+    assert(a == [P.init, P.init, P.init]);


Needs tests for const, e.g

static struct S { float x = 0; float y = 0; } static foreach (e; ["mutable", "non-mutable"]) {{ static if (e == "mutable") alias P = S; else alias P = ConstOf!S; static assert(isInitAllZeroBits!P); P[] a = [P(10, 11), P(20, 21), P(40, 41)]; uninitializedFillDefault(a); assert(a == [P.init, P.init, P.init]); }}

fails with:

std/experimental/allocator/package.d(929): Error: function core.stdc.string.memset(return scope void* s, int c, ulong n) is not callable using argument types (const(S)*, int, ulong) std/experimental/allocator/package.d(994): Error: template instance std.experimental.allocator.uninitializedFillDefault!(const(S)) error instantiating

Shouldn't trying to write to const without casting const-ness away result in compilation failure?

EDIT (x2): When this function is used in makeArray const-ness is cast away by the caller. So I think not removing const-ness inside uninitializedFillDefault is consistent with the current design.

phobos/std/experimental/allocator/package.d

Lines 1453 to 1454 in c7c83e6

alias U = Unqual!T;

return () @trusted { return cast(T[]) uninitializedFillDefault(cast(U[]) m); }();

PetarKirov · 2018-01-14T21:33:12Z

std/experimental/allocator/common.d

+package template isInitAllZeroBits(T)
+{
+    import std.traits : isStaticArray;
+    static if (isStaticArray!T)


I would use a more generic trait: static if (T.sizeof == 0).

apz28 · 2018-02-13T16:00:31Z

Improvement codes, loop should not be continue if isAllZeroBits return false.

       static foreach (i; 0 .. T.length)

           b &= isAllZeroBits!(typeof(value[i]), value[i]);

n8sh · 2018-02-15T22:34:18Z

~~@apz28 The problem is because I am using static foreach if I return before the end the compiler will complain about unreachable code.~~

n8sh · 2018-02-16T00:25:42Z

@apz28 I've fixed the issue you raised.

n8sh · 2018-03-14T22:19:53Z

Ping @wilzbach @JackStouffer @ZombineDev @DmitryOlshansky.

wilzbach

Noice!

wilzbach · 2018-03-28T01:47:52Z

Putting this on the merge queue (if no objections are raised in the next three days, this can be merged).

Applies to code that was removed.

wilzbach · 2018-03-28T01:51:32Z

BTW @n8sh we have this semi-official Slack channel that we often for some quick discussion. I couldn't find your email address, so if you want to join, just send me an email ;-)

nordlow · 2018-03-28T09:38:07Z

std/experimental/allocator/common.d

+    static assert(isAllOneBits!(char, 0xff));
+    static assert(isAllOneBits!(wchar, 0xffff));
+    static assert(isAllOneBits!(byte, cast(byte) 0xff));
+    static assert(isAllOneBits!(int, 0xffff_ffff));


I would prefer to see tests for all integer types here just to be sure.

When we can statically determine that the representation of T.init consists of nothing but zeroes or nothing but ones we use memset. (The second case occurs for char and wchar.)

n8sh requested review from PetarKirov, andralex and wilzbach as code owners January 11, 2018 06:38

dlang-bot added the Severity:Enhancement label Jan 11, 2018

n8sh commented Jan 11, 2018

View reviewed changes

n8sh force-pushed the uninitializedFillDefault-memset branch from 554a6f2 to bbf8757 Compare January 11, 2018 06:45

n8sh commented Jan 11, 2018

View reviewed changes

n8sh force-pushed the uninitializedFillDefault-memset branch 3 times, most recently from 7f6c48c to 53c3865 Compare January 11, 2018 08:11

DmitryOlshansky previously requested changes Jan 11, 2018

View reviewed changes

n8sh force-pushed the uninitializedFillDefault-memset branch 3 times, most recently from ed54668 to 4cfb52e Compare January 11, 2018 23:39

n8sh force-pushed the uninitializedFillDefault-memset branch 2 times, most recently from bdfe933 to 5a0ef22 Compare January 12, 2018 02:29

wilzbach reviewed Jan 12, 2018

View reviewed changes

n8sh force-pushed the uninitializedFillDefault-memset branch 4 times, most recently from 8038986 to 779cf53 Compare January 12, 2018 15:24

wilzbach mentioned this pull request Jan 14, 2018

avoid Buffer type which creates a huge __initZ symbol vibe-d/vibe-core#49

Merged

wilzbach reviewed Jan 14, 2018

View reviewed changes

PetarKirov reviewed Jan 14, 2018

View reviewed changes

n8sh force-pushed the uninitializedFillDefault-memset branch 2 times, most recently from 5f9a7c6 to 6fb050c Compare January 15, 2018 17:12

n8sh force-pushed the uninitializedFillDefault-memset branch 3 times, most recently from a072bf8 to fc69535 Compare February 12, 2018 18:43

n8sh force-pushed the uninitializedFillDefault-memset branch from fc69535 to 192220f Compare February 16, 2018 00:25

n8sh force-pushed the uninitializedFillDefault-memset branch 6 times, most recently from 6296021 to b8c2b01 Compare March 14, 2018 21:01

JackStouffer added the Review:Needs Review label Mar 16, 2018

wilzbach approved these changes Mar 28, 2018

View reviewed changes

wilzbach added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Mar 28, 2018

wilzbach added this to the 2.080.0 milestone Mar 28, 2018

nordlow reviewed Mar 28, 2018

View reviewed changes

n8sh force-pushed the uninitializedFillDefault-memset branch 2 times, most recently from d52186b to aa8f45a Compare March 28, 2018 17:20

Fix Issue 18223: use memset in uninitializedFillDefault(T[])

8777cbf

When we can statically determine that the representation of T.init consists of nothing but zeroes or nothing but ones we use memset. (The second case occurs for char and wchar.)

n8sh force-pushed the uninitializedFillDefault-memset branch from aa8f45a to 8777cbf Compare March 28, 2018 17:59

n8sh added Merge:auto-merge and removed Review:Needs Review Merge:72h no objection -> merge The PR will be merged if there are no objections raised. labels Mar 31, 2018

dlang-bot merged commit 6797b52 into dlang:master Mar 31, 2018

	alias U = Unqual!T;
	return () @trusted { return cast(T[]) uninitializedFillDefault(cast(U[]) m); }();

Uh oh!

Conversation

n8sh commented Jan 11, 2018

Uh oh!

dlang-bot commented Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Testing this PR locally

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n8sh commented Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n8sh Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n8sh Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apz28 commented Feb 13, 2018

Uh oh!

n8sh commented Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n8sh commented Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n8sh commented Mar 14, 2018

dlang-bot commented Jan 11, 2018 •

edited

Loading

n8sh commented Jan 11, 2018 •

edited

Loading

wilzbach Jan 12, 2018 •

edited

Loading

n8sh Jan 15, 2018 •

edited

Loading

n8sh Jan 15, 2018 •

edited

Loading

n8sh commented Feb 15, 2018 •

edited

Loading

n8sh commented Feb 16, 2018 •

edited

Loading