Extend std.range.chunks to work with non-forward input ranges. by quickfur · Pull Request #5624 · dlang/phobos

quickfur · 2017-07-17T20:58:43Z

Rationale: sometimes all you can get is an input range (e.g., File.byLine), and it is undesirable to have to buffer the entire input range just to be able to call chunks on it. This extension of chunks to support non-forward input ranges allows for the possibility of caching such ranges by chunks so that you process it incrementally while offering better range primitives on the cached portion of the range.

For example:

File("data").byLineCopy
    .chunks(bufSize)
    .map!(chunk => chunk.array) // cache each chunk on a buffer
    ... // now each chunk is a forward range and
        // you can do more stuff with it
        // Even though outer range is still only input range

For now, I'm implementing this as a separate overload of Chunks, due to the unfortunate historical accident that Chunks was exposed as a public API rather than encapsulated as a Voldemort or private module-global struct. The implementation is fundamentally different, since without forward range primitives the current code simply cannot be made to work correctly. Originally I tried to use static if inside the struct to separate the two implementations, but it was very messy and the diff was a sight to behold. So I'm keeping it as a separate overload for now.

dlang-bot · 2017-07-17T20:58:44Z

Thanks for your pull request, @quickfur!

Bugzilla references

Auto-close	Bugzilla	Description
✓	15759	chunks should work with only an input range

wilzbach

Nice! I needed exactly this a while ago :)
So you can add "Fix 15759" to the commit message.

I made a few comments on my first round.

wilzbach · 2017-07-17T21:11:10Z

std/range/package.d

+    private size_t curSizeLeft;
+    bool empty;
+
+    this(Source _r, size_t _chunkSize)


I think these parameter are publicly exposed, so I think it's better to use this.x = x below (even if it's a bit uglier).

No longer applicable in the new code.

wilzbach · 2017-07-17T21:13:38Z

std/range/package.d

+    private Source r;
+    private size_t chunkSize;
+    private size_t curSizeLeft;
+    bool empty;


This is writable by the user -> should be private and wrapped .

wilzbach · 2017-07-17T21:19:44Z

std/range/package.d

+    {
+        private Chunks* impl;
+
+        @property bool empty() { return impl.curSizeLeft == 0 || impl.r.empty; }


Could be const if impl.r.empty is const. This is typically checked with sth. like:

static if (is(typeof((cast(const typeof(impl.r))impl.r).empty)))

but it's not very clean and thus not done at many places in Phobos ...

Doesn't dmd infer attributes on these methods, since the outer struct is a template?

wilzbach · 2017-07-17T21:24:48Z

std/range/package.d

+        empty = r.empty;
+    }
+
+    @property Chunk front() { return Chunk(&this); }


I'm a bit worried about this. Won't the wrapping Chunks get deconstructed when you leave the scope, e.g. by returning its front?
(we should at least test this)

Haha, just saw this comment... but this is already causing a breakage when compiled with -dip25 -dip1000 because the new isInputRange tests front with typeof((R r) => r.front), which will escape the reference to this, and causes it to reject this as being a range.

I suppose I should turn this into a class rather than a struct... in the original non-Phobos version of this in my own code, the wrapping Chunks is always allocated on the heap. The problem with -dip25 -dip1000 still exists, though.

OK, just reworked the code a bit to move the implementation bits into a heap-allocated struct, so that the outer Chunks is just a wrapper around a pointer to it. This makes it cleaner and also fixes the problems with -dip25 -dip1000.

wilzbach · 2017-07-17T21:36:46Z

due to the unfortunate historical accident that Chunks was exposed as a public API rather than encapsulated as a Voldemort or private module-global struct.

Btw two simple things that we could do to improve the status quo:

add the missing options (e.g. SearchPolicy, TransverseOptions) to bookmark table (or create a new one) and hide the quick index
undocument the publicly exposed structs, s.t. at least the docs look nicer for newcomers

Opinions?

quickfur · 2017-07-17T22:20:36Z

Sounds like a good idea, kill off the autoindex (which is very ugly anyway) and undocument the wrapper structs. Don't make them private just yet, to avoid breaking any existing code that may actually refer to their names. But hide them from the docs so that no new code will continue to rely on them.

wilzbach · 2017-07-17T22:56:01Z

Sounds like a good idea, kill off the autoindex (which is very ugly anyway) and undocument the wrapper structs. Don't make them private just yet, to avoid breaking any existing code that may actually refer to their names. But hide them from the docs so that no new code will continue to rely on them.

Great that we are in agreement -> #5625

JackStouffer · 2017-07-18T02:49:15Z

std/range/package.d


+@safe unittest
+{
+    import std.algorithm.comparison : equal;


import std.internal.test.dummyrange : ReferenceInputRange; auto data = [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; auto r = new ReferenceInputRange!int(data);

JackStouffer · 2017-07-18T02:54:03Z

std/range/package.d

+
+    private this(Source r, size_t chunkSize)
+    {
+        impl = new Impl(r, chunkSize, chunkSize);


Is putting this on the heap really nessesary? It's just that you're throwing out @nogc.

It's a shame that RefCounted is still unsafe...

I'm on the fence about this. Ideally, we should use RefCounted here, I think byChunk does that, but then it's a toss-up between whether you want to keep @nogc or @safe, but you can't have both.

I'm OK either way -- if you guys feel it's more consistent to use RefCounted ala byChunk, then it's a relatively easy change.

Two more arguments for RefCounted:

making safe RC (core.rc) is on the agenda, so if we use RefCounted now, chances are good that it might turn @safe in the future

it's easier to use @trusted on your code than the hacky and rather unknown assumeNoGc.
In any case it's probably a good idea to open an issue about it, s.t. it's not forgotten.

JackStouffer · 2017-07-18T03:01:19Z

std/range/package.d

 fewer than $(D chunkSize) elements.

+If `Source` is an input range but not a forward range, the resulting range and
+chunks will be single-pass only: iterating over `front` will shrink the chunk


IMO there's no need for the long explaination; people should already know what an input range is if they're reading this

If Source is an input range and not a forward range, each chunk will also be an input range. Any calls to popFront on Chunks will also invalidate any lingering references to previous values in each chunk.

Well, I just wanted to make sure people come with no expectations that we will cache range elements or otherwise make the resulting range "better behaved". A lot of Phobos algorithms will act funny when given a non-forward input range (try it sometime), and I have heard complaints in the past about why something doesn't behave "intuitively", even though that is impossible given input range semantics.

"If Source is a forward range, the resulting chunks will be forward ranges as well. Otherwise (i.e. Source is an input range), the resulting chunks will be input ranges consuming the same input."

JackStouffer · 2017-07-18T03:17:11Z

std/range/package.d

@@ -7147,6 +7208,43 @@ if (isForwardRange!Source)
    assert(equal(retro(array(chunks)), array(retro(chunks))));


I would add the following example to show the new functionality

int i; auto inputRange = generate!(() => ++i).take(10); auto chunked = inputRange.chunks(2); assert(inputRange.front.equal([1, 2])); assert(inputRange.front.empty); inputRange.popFront; assert(inputRange.front.equal([3, 4]));

or something similar

quickfur · 2017-07-19T21:21:04Z

Hmm, I tried converting the heap allocation to use RefCounted instead, but got this error:

std/conv.d(4366): Error: static assert  "Impl cannot be emplaced from (RefCounted!(Impl, cast(RefCountedAutoInitialize)1)*)."
std/conv.d(4447):        instantiated from here: emplaceRef!(Impl, Impl, RefCounted!(Impl, cast(RefCountedAutoInitialize)1)*)
std/typecons.d(5221):        instantiated from here: emplace!(Impl, RefCounted!(Impl, cast(RefCountedAutoInitialize)1)*)
std/typecons.d(5312):        instantiated from here: initialize!(RefCounted!(Impl, cast(RefCountedAutoInitialize)1)*)
std/range/package.d(7160):        ... (1 instantiations, -v to show) ...
std/range/package.d(7185):        instantiated from here: Chunks!(Take!(Generator!(delegate () => i += 1)))
std/range/package.d(7217):        instantiated from here: chunks!(Take!(Generator!(delegate () => i += 1)))

Any ideas what might be wrong?

JackStouffer · 2017-07-20T05:24:00Z

TBH when I asked about putting it on the heap, I was talking about in general and not about using the GC. Is there a specific reason why you're using an indirection here?

quickfur · 2017-07-21T00:29:41Z

Very good question. The original code I drew this from actually did not have the indirection... but I quickly discovered that mixing non-forward input ranges with by-value semantics was a very bad combination, because in a complex UFCS pipeline, you easily end up inadvertently passing part of the state by value, thus causing the range wrapper to go out of sync with the input range being wrapped (esp. when the latter has by-reference semantics, or references external state). After hacking the code with various workarounds, eventually I just gave up and decided that the cleanest way to guarantee correct behaviour was to simply make the whole thing by-reference, including the Chunks.

Even that turned out to be insufficient in the face of structs' pass-by-value semantics, so eventually all state has to be stored in the wrapper's state, and Chunk itself became basically just a wrapper to this "mothership" wrapper in order to work in all cases.

quickfur · 2017-07-21T00:33:20Z

I'd very much like the RefCounted version to be merged, actually. Following groupBy's pattern, I think this is a better solution in terms of the general direction of making Phobos @nogc. If I can only figure out why RefCounted is asserting... :-(

quickfur · 2017-07-21T00:42:01Z

Gah, I'm an idiot. Was trying to assign new RefCounted!Impl(...) to impl... forgot to take out the "new" (what was I thinking?!). Works fine now, except the unittests are no longer @safe.

JackStouffer · 2017-07-21T01:51:56Z

Hmm, do we want @safe or @nogc?

quickfur · 2017-07-21T01:54:17Z

Haha, that was also my reaction. :-P Ideally both, but that will have to wait for RefCounted to become @safe. I haven't been following recent developments on that front, though... not sure what exactly is involved or how soon we can expect it.

wilzbach · 2017-07-21T01:56:13Z

Hmm, do we want @safe or @nogc?

Copy/pasting my PoV from the sub-comments:

making safe RC (core.rc) is on the agenda, so if we use RefCounted now, chances are good that it might turn @safe in the future
it's easier to use @trusted on your code than the hacky and rather unknown assumeNoGc

JackStouffer · 2017-07-21T01:59:07Z

not sure what exactly is involved or how soon we can expect it.

IIRC it requires the DIP1000 stuff to be fully working

andralex · 2017-07-25T17:37:51Z

std/range/package.d

 fewer than $(D chunkSize) elements.

+If `Source` is an input range but not a forward range, the resulting range and
+chunks will be single-pass only: iterating over `front` will shrink the chunk


"If Source is a forward range, the resulting chunks will be forward ranges as well. Otherwise (i.e. Source is an input range), the resulting chunks will be input ranges consuming the same input."

andralex · 2017-07-25T17:42:19Z

std/range/package.d


-Returns: Forward range of all chunks with propagated bidirectionality,
-         conditional random access and slicing.
+Returns: Input range of all chunks with propagated forwardness,


"forwardness" is a bit awkward, rephrase? Just say what you mean, you've already explained the details: "Returns: Range of chunks"

andralex · 2017-07-25T17:43:55Z

std/range/package.d

+    bidirectionality, conditional random access and slicing.
 */
 struct Chunks(Source)
 if (isForwardRange!Source)


The fact that implementation is distinct for input and forward ranges is an implementation detail. Use the weakest constraint here and use static if inside the struct to fork the implementation.

Originally, this is what I did but it made the diff look really ugly, that's why I went with splitting it into two overloads. But since you asked for it, I'll go back to doing it that way.

andralex · 2017-07-25T17:44:30Z

std/range/package.d

    size_t _chunkSize;
 }

+/// ditto


merge with the struct above

andralex · 2017-07-25T17:46:12Z

std/range/package.d

+        private Source r;
+        private size_t chunkSize;
+        private size_t curSizeLeft;
+        private bool _empty;


why the underscore

To prevent being overwritten from outside, as pointed out by @wilzbach's review.

andralex · 2017-07-25T17:48:19Z

std/range/package.d

+    private this(Source r, size_t chunkSize)
+    {
+        impl = RefCounted!Impl(r, chunkSize, chunkSize);
+        impl._empty = r.empty;


does impl = RefCounted!Impl(r, chunkSize, chunkSize, r.empty); work?

andralex · 2017-07-25T17:50:38Z

std/range/package.d

+        {
+            impl.curSizeLeft--;
+            impl.r.popFront();
+        }


Is this impl.curSizeLeft -= impl.r.popFrontN(impl.curSizeLeft);?

andralex · 2017-07-25T17:52:06Z

std/range/package.d

+        if (!impl.r.empty)
+            impl.curSizeLeft = impl.chunkSize;
+        else
+            impl._empty = true;


Looks like impl._empty is just a cache for impl.r.empty, why not eliminate? Alternatively, put a 0 in impl.chunkSize when done.

Can't eliminate it because the original range may have been exhausted after the last chunk is iterated, but we still need to allow one more call to popFront to pop off the last chunk (conceptually, of course it's a no-op).

But your idea of setting impl.chunkSize to 0 when empty is a good one, it lets us eliminate _empty after all. I'll go with that.

quickfur · 2017-07-26T20:48:08Z

@andralex The diff now looks rather ugly, but if you add ?w=1 to the Github URL, it will suppress whitespace diffs and will look cleaner. Here's the link.

andralex

thx for a solid piece of work

andralex · 2017-07-26T23:54:07Z

please submit a changelog entry too

JackStouffer · 2017-07-31T19:50:47Z

std/range/package.d

 }

+/// Non-forward input ranges are supported, but with limited semantics.
+/*@safe*/ unittest // FIXME: can't be @safe because RefCounted isn't.


Ping @quickfur, this needs to be marked @system in order to pass circle.

JackStouffer · 2017-07-31T19:50:53Z

std/range/package.d

+    assert(chunked.front.equal([3, 4]));
+}
+
+/*@safe*/ unittest


JackStouffer · 2017-08-01T02:08:41Z

changelog/std-range-input-chunks.dd

+`std.range.chunks` was extended to support non-forward input ranges.
+
+Now `std.range.chunks` can be used with input ranges that are not forward
+ranges, albeit with limited semantics as imposed by the underlying range. 


@quickfur Trailing whitespace

…ut ranges. Add input range example.

quickfur requested review from JackStouffer, PetarKirov, andralex and wilzbach as code owners July 17, 2017 20:58

wilzbach reviewed Jul 17, 2017

View reviewed changes

quickfur force-pushed the input-chunks branch from 3f1cfa5 to 235ab26 Compare July 17, 2017 22:14

quickfur force-pushed the input-chunks branch from 235ab26 to da3b27e Compare July 17, 2017 22:30

wilzbach mentioned this pull request Jul 17, 2017

Undocument public structs in std.range #5625

Closed

JackStouffer reviewed Jul 18, 2017

View reviewed changes

quickfur force-pushed the input-chunks branch from aa6c0ca to a870933 Compare July 18, 2017 22:54

andralex requested changes Jul 25, 2017

View reviewed changes

quickfur force-pushed the input-chunks branch from 90b237e to e490bd8 Compare July 26, 2017 20:41

dlang-bot added the Severity:Enhancement label Jul 26, 2017

andralex approved these changes Jul 26, 2017

View reviewed changes

andralex added the Merge:auto-merge label Jul 26, 2017

JackStouffer reviewed Jul 31, 2017

View reviewed changes

quickfur force-pushed the input-chunks branch from e490bd8 to f46091e Compare July 31, 2017 21:02

dlang-bot removed the Merge:auto-merge label Jul 31, 2017

JackStouffer added the Merge:auto-merge label Aug 1, 2017

JackStouffer reviewed Aug 1, 2017

View reviewed changes

H. S. Teoh added 2 commits August 1, 2017 14:01

Fix issue 15759: Extend std.range.chunks to work with non-forward inp…

6826427

…ut ranges. Add input range example.

Add changelog entry.

c1d7490

quickfur force-pushed the input-chunks branch from f46091e to c1d7490 Compare August 1, 2017 21:09

dlang-bot removed the Merge:auto-merge label Aug 1, 2017

JackStouffer added the Merge:auto-merge label Aug 1, 2017

dlang-bot merged commit 08e3703 into dlang:master Aug 1, 2017

quickfur deleted the input-chunks branch August 1, 2017 22:07

quickfur mentioned this pull request Aug 10, 2017

Don't use alias this in chunks implementation. #5689

Merged

		@@ -7147,6 +7208,43 @@ if (isForwardRange!Source)
		assert(equal(retro(array(chunks)), array(retro(chunks))));

Uh oh!

Conversation

quickfur commented Jul 17, 2017

Uh oh!

dlang-bot commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Uh oh!

wilzbach left a comment

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!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Jul 17, 2017

Uh oh!

quickfur commented Jul 17, 2017

Uh oh!

wilzbach commented Jul 17, 2017

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!

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!

quickfur commented Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JackStouffer commented Jul 20, 2017

Uh oh!

quickfur commented Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

quickfur commented Jul 21, 2017

Uh oh!

quickfur commented Jul 21, 2017

Uh oh!

JackStouffer commented Jul 21, 2017

Uh oh!

quickfur commented Jul 21, 2017

Uh oh!

wilzbach commented Jul 21, 2017

Uh oh!

JackStouffer commented Jul 21, 2017

Uh oh!

Choose a reason for hiding this comment

dlang-bot commented Jul 17, 2017 •

edited

Loading

quickfur commented Jul 19, 2017 •

edited

Loading

quickfur commented Jul 21, 2017 •

edited

Loading