RandomCover use a `@nogc` buffer to store already chosen results #6576

ghost · 2018-06-10T13:23:12Z

The life time of the array storing the already chosen results is deterministic so there's no need to use a built-in array, instead a small dedicated array is used. This change is far from making the randomCover family of functions @nogc but at least it avoids an alloc (fundamentally, issue 14001 is blocked by exceptions in uniform and in the different RNGs).

dlang-bot · 2018-06-10T13:23:13Z

Thanks for your pull request and interest in making D better, @bbasile! 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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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#6576"

wilzbach

I see the motivation, but it somehow feels like we are re-inventing the wheel.
There shouldn't be a need to code a custom struct every time we want sth. to be @nogc :/
Could we at least maybe use stdx.allocator?

wilzbach · 2018-06-10T15:42:16Z

std/random.d

+        void* nbuffer = malloc(_length);
+        if (nbuffer is null)
+            assert(0, emsg);
+        buffer = memmove(nbuffer, buffer, _length);


why not makeArray?

ghost · 2018-06-10T16:21:41Z

Could we at least maybe use stdx.allocator?

I thought to this and concluded that it's not a good idea to use the allocators in phobos until their are promoted to std. (note that they are not used at all in phobos).

There shouldn't be a need to code a custom struct every time we want sth. to be @nogc

Yeah. Obviously i'd use std.container.array.Array if it hasn't this hilarious constraint that is, every type is allowed as element but not bool.

wilzbach · 2018-06-10T16:31:58Z

I thought to this and concluded that it's not a good idea to use the allocators in phobos until their are promoted to std. (note that they are not used at all in phobos).

FWIW that shouldn't be a problem and for example std.file is using std.experimental.checkedint already.
As long as it's used internally, we should be good. Imho it's even good if we start to use it internally because only by intensive use we can detect flaws.

Obviously i'd use std.container.array.Array if it hasn't this hilarious constraint that is, every type is allowed as element but not bool.

Ouch. Does anyone know about the history of this constraint?
Was the idea that BitArray is a replacement?

aG0aep6G · 2018-06-10T16:45:41Z

Yeah. Obviously i'd use std.container.array.Array if it hasn't this hilarious constraint that is, every type is allowed as element but not bool.

The constraint is just on the first template, because there's another one that's specialized for bool: https://dlang.org/phobos/std_container_array.html#.Array.2

It would probably be less confusing if there was just one template, with the specialization inside.

ghost · 2018-06-10T18:36:16Z

Array!bool is not @nogc enough anyway, there are plenty of compile errors when used.

ghost · 2018-06-10T19:43:30Z

The new custom type can be made more useful by allowing bit packed array of bool for small length (up to size_t * 8 elements to cover.

… is used

ghost · 2018-06-12T08:15:02Z

@wilzbach, maybe you could reconsider your POV. Now that a field of bits is used the custom type is really interesting for small (<= 64) ranges to cover.

n8sh · 2018-06-13T01:17:02Z

std/random.d

+
+    this(this) nothrow @nogc @trusted
+    {
+        import core.stdc.stdlib : malloc;


core.memory : pureMalloc, pureCalloc, pureFree?

Although since you assert(0) if they fail, it would be justifiable to call "locally pureified" malloc/etc. directly.

i've moved to onOutOfMemoryErrorNoGC.
Now there's a dilemma : pure allocs and asserts or static exceptions and impure allocs

onOutOfMemoryError is @nogc nothrow pure, so why not use it instead of onOutOfMemoryErrorNoGC?

n8sh · 2018-06-13T01:45:27Z

std/random.d

+        {
+            void* nbuffer = malloc(_length);
+            if (nbuffer is null)
+                assert(0, emsg);


In places like this it seems like the convention is to use onOutOfMemoryError:

if (nbuffer is null) { onOutOfMemoryError(); assert(0); }

...but this has the downside that your custom message would be lost.

n8sh · 2018-06-13T01:47:45Z

std/random.d

+            if (nbuffer is null)
+                assert(0, emsg);
+            buffer = memmove(nbuffer, buffer, _length);
+            if (buffer is null)


This check doesn't make sense. Cut & paste error?

n8sh · 2018-06-13T05:07:21Z

std/random.d

+        }
+    }
+
+    this(size_t value) nothrow @nogc @trusted @property


Parameter name should hint at purpose.

ghost · 2018-06-13T09:27:28Z

Actually i think this could close 14001. While RandomCover is still not @nogc every suggestion made in there is implemented.

n8sh · 2018-06-13T14:30:39Z

std/random.d

+    private immutable size_t _length;
+    private immutable bool hasPackedBits;
+
+    this(this) pure nothrow @nogc @trusted


I believe an explicit (or disabled) opAssign is also required.

n8sh · 2018-06-13T14:32:21Z

std/random.d

+            void* nbuffer = pureMalloc(_length);
+            if (nbuffer is null)
+                onOutOfMemoryError();
+            buffer = memmove(nbuffer, buffer, _length);


You can use memcpy here instead of memmove because the destination and source cannot overlap.

wilzbach

FWIW I didn't object to this PR, I just said that it would be nice if we could be reusing existing containers (instead of rolling yet another custom-built one).
I had a brief look at the bool-version of Array and I agree that it would take quite some effort to make it as efficient/specialized like this container.
Also, hopefully the new collections library gets into stdx soon, s.t. we can use these containers in the future ...

ghost requested a review from wilzbach as a code owner June 10, 2018 13:23

RandomCover use a @nogc buffer to store already chosen results

a8869e2

wilzbach reviewed Jun 10, 2018

View reviewed changes

use a bit packed vector (w/o heap alloc) for small lengths

395e5e9

Basile Burg added 3 commits June 11, 2018 06:04

make impossible to change length and use a var to test if a bit field…

87a909c

… is used

fine tweaks

acb4811

cover postblit when heap storage is used

7649147

n8sh reviewed Jun 13, 2018

View reviewed changes

address review: use static OOME and remove useless null check

047f727

n8sh reviewed Jun 13, 2018

View reviewed changes

std/random.d Outdated

}

}

this(size_t value) nothrow @nogc @trusted @property

Copy link

Member

n8sh Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parameter name should hint at purpose.

address review: make the custom array pure

4b3d0ed

remove wrong ddoc and make disable default ctor

d3a933b

n8sh reviewed Jun 13, 2018

View reviewed changes

address review: use memcpy and disable assignment

a716ad6

wilzbach approved these changes Jun 18, 2018

View reviewed changes

n8sh added 72h no objection -> merge The PR will be merged if there are no objections raised. optimization auto-merge labels Jul 10, 2018

dlang-bot merged commit c7b1cf1 into dlang:master Jul 15, 2018

n8sh mentioned this pull request Aug 10, 2018

Fix Issue 14001 & Issue 19156 - @nogc std.random.randomCover and std.random.randomShuffle #6657

Merged

ghost deleted the rnd-cov-nogc-buff branch December 1, 2018 01:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RandomCover use a `@nogc` buffer to store already chosen results #6576

RandomCover use a `@nogc` buffer to store already chosen results #6576

ghost commented Jun 10, 2018

dlang-bot commented Jun 10, 2018

wilzbach left a comment

wilzbach Jun 10, 2018

ghost commented Jun 10, 2018

wilzbach commented Jun 10, 2018

aG0aep6G commented Jun 10, 2018

ghost commented Jun 10, 2018 •

edited by ghost

ghost commented Jun 10, 2018

ghost commented Jun 12, 2018

n8sh Jun 13, 2018

n8sh Jun 13, 2018

ghost Jun 13, 2018 •

edited by ghost

n8sh Jun 13, 2018

ghost Jun 13, 2018

n8sh Jun 13, 2018

n8sh Jun 13, 2018

n8sh Jun 13, 2018

ghost commented Jun 13, 2018

n8sh Jun 13, 2018

n8sh Jun 13, 2018

wilzbach left a comment

RandomCover use a @nogc buffer to store already chosen results #6576

RandomCover use a @nogc buffer to store already chosen results #6576

Conversation

ghost commented Jun 10, 2018

dlang-bot commented Jun 10, 2018

Bugzilla references

Testing this PR locally

wilzbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 10, 2018

wilzbach commented Jun 10, 2018

aG0aep6G commented Jun 10, 2018

ghost commented Jun 10, 2018 • edited by ghost

ghost commented Jun 10, 2018

ghost commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jun 13, 2018 • edited by ghost

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

ghost commented Jun 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach left a comment

Choose a reason for hiding this comment

RandomCover use a `@nogc` buffer to store already chosen results #6576

RandomCover use a `@nogc` buffer to store already chosen results #6576

ghost commented Jun 10, 2018 •

edited by ghost

ghost Jun 13, 2018 •

edited by ghost