Fix issue 17019: `each` should be usable with parallel (and behave like foreach) by wilzbach · Pull Request #4990 · dlang/phobos

wilzbach · 2016-12-23T01:29:00Z

The following test fails:

unittest
{
    import std.algorithm.iteration : each, sum;
    import std.parallelism : parallel;
    import std.range : iota;

    auto logsIndex = new int[10];
    parallel(logsIndex).each!((i, ref e) => e += i);
    assert(logsIndex.sum == 10.iota.sum);
}

while it passes without parallel:

unittest
{
    import std.algorithm.iteration : each, sum;
    import std.parallelism : parallel;
    import std.range : iota;

    auto logsIndex = new int[10];
    logsIndex.each!((i, ref e) => e += i);
    assert(logsIndex.sum == 10.iota.sum);
}

of course the foreach alternative works as well

unittest
{
    import std.algorithm.iteration : sum;
    import std.parallelism : parallel;
    import std.range : iota;

    auto logsIndex = new int[10];
    foreach (i, ref e; parallel(logsIndex))
        e += i;
    assert(logsIndex.sum == 10.iota.sum);
}

(I also added more tests, which should hopefully help to cover more edge cases)

dlang-bot · 2016-12-23T01:29:01Z

Thanks for your pull request, @wilzbach!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	17019	enhancement	std.algorithm.iteration.each should be usable with parallel

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

PetarKirov

Overall LGTM, just some minor comments below:

PetarKirov · 2016-12-24T17:21:28Z

std/algorithm/iteration.d

+            foreach (ref a, ref b; r)
+                cast(void)binaryFun!BinaryArgs(a, b);
+        }
+        else // static if (isForeachUnaryWithIndexIterable!Iterable)


If you want, you can change the static if to a static assert and move it into the else block.

Oh I opted for this because it was used like this before. I prefer static assert as well :)

PetarKirov · 2016-12-24T17:22:38Z

std/algorithm/iteration.d

        debug(each) pragma(msg, "Using foreach for ", Iterable.stringof);
        static if (isForeachUnaryIterable!Iterable)
        {
+            debug(each) pragma(msg, "unary");


I guess this can be removed, now that you have debugged the issue ;)

PetarKirov · 2016-12-24T17:24:21Z

std/algorithm/iteration.d

            foreach (ref e; r)
+            {
                cast(void)unaryFun!pred(e);
+            }


Nit/personal preference: curly braces are not needed for simple singe statement blocks.

PetarKirov · 2016-12-24T17:28:29Z

std/algorithm/iteration.d

+/// each should work with index
+@safe unittest
+{
+    import std.range : iota, lockstep;


I think you also need to import sum, even though it's in same module, so the unittest can be run in the online documentation.

even though it's in same module, so the unittest can be run in the online documentation.

Luckily only external dependencies (e.g. global imports), it's fairly trivial to a import x.y.x and we do so:

https://github.com/dlang/dlang.org/blob/master/js/run_examples.js#L29

The automatic tests extractor which is partially enabled for Phobos does this as well and will warn if someone tries to sneak public tests in that aren't runnable ;-)

Nice! LGTM then.

PetarKirov · 2016-12-24T17:35:10Z

std/algorithm/iteration.d

 }

+// #15357: `each` should behave similar to foreach
+/// each should work with index


Since it's a ddoc-ed example, "each works with iterable objects which provide an index variable, along with each element", or sth. like that would be more appropriate.

PetarKirov · 2016-12-24T17:39:34Z

std/algorithm/iteration.d

+    assert(arrA.sum == 0);
+    assert(arrB.sum == 4);
+
+    /// three ref parameters


Nit: use either numbers or words on both places for consistency.

PetarKirov · 2016-12-24T17:48:40Z

std/algorithm/iteration.d

+    parallel(logs).each!((ref e) => e += 1);
+    assert(logs.sum == 10);
+
+    auto logsIndex = new int[10];


Nit: s/logs/arr/ (logs sounds confusing).

JackStouffer · 2017-01-11T08:04:04Z

std/algorithm/iteration.d

-            foreach (ref i, ref a; r)
-                cast(void)binaryFun!BinaryArgs(i, a);
+            foreach (ref a, ref b; r)
+                cast(void)binaryFun!BinaryArgs(a, b);


rationale for changing the parameter names?

Strange, yes, but you can't be perfect. I think we should pull.

To symbolize that it's a binary iteration with two elements - i is usually used as designated variable for the loop index.
The idea was to make it similar to std.functional.binaryFun.

Moreover, the private function isForeachBinaryIterable got renamed to isForeachUnaryWithIndexIterable (above) for which the loop variable names are kept.

dukc

each should be usable with everything foreach is with, yes. If only language makes it possible.

More than enough unittests are provided and everything passes. We should pull it.

wilzbach · 2017-02-16T01:51:46Z

(rebased to latest upstream/master)

wilzbach · 2017-02-28T18:04:43Z

That's weird. It seems for Windows this creates a cyclic dependency:

object.Error@src\rt\minfo.d(378): Cyclic dependency between module std.internal.math.biguintcore and std.parallelism
std.internal.math.biguintcore* ->
std.traits ->
std.algorithm.iteration ->
std.parallelism* ->
std.algorithm.iteration ->
std.bigint ->
std.internal.math.biguintcore*

CC @schveiguy any easy trick to workaround this?

schveiguy · 2017-02-28T18:18:02Z

@wilzbach I've run into that before. I think it's due to a difference in how the module info is output on non-Windows platforms. I never got around to figuring out why it happens. The cycle is definitely there, and it's not Windows-specific.

How you deal with the cycle is to get rid of the cycle, no easy tricks. If I were to approach it, I'd say try moving the CACHELIMIT variable and its initialization into its own file. That should break the cycle, since it only depends on druntime, so you can't have a cycle back to Phobos through core.cpuid.

schveiguy · 2017-02-28T18:37:01Z

I've run into that before

FYI: dlang/druntime#1602 (comment)

wilzbach · 2018-02-07T00:30:20Z

Rebased to master. I have good hopes that this finally passes the testsuite as there was quite some work to get rid of the shared module constructors...

wilzbach · 2018-02-07T01:17:08Z

Yeah, sometimes time does heal PRs 🎉

wilzbach · 2018-04-03T07:10:53Z

Yeah, sometimes time does heal PRs tada

Grr... and sometime time does break the PR again.
This time it's a segfault with -dip1000 (which is used to build this module since a few weeks):

Reduced ICE: https://issues.dlang.org/show_bug.cgi?id=18713

wilzbach · 2018-06-06T16:44:35Z

Now that we no longer use -dip1000, this passes again:

…parallel

wilzbach · 2018-08-14T13:54:27Z

Can't be more green than now. Ready to move on?

andralex

nice work

wilzbach force-pushed the fix-15357-each-foreach branch 2 times, most recently from ea4cc19 to 97a66cc Compare December 23, 2016 22:48

PetarKirov approved these changes Dec 24, 2016

View reviewed changes

wilzbach force-pushed the fix-15357-each-foreach branch from 7a61f31 to 7273b50 Compare December 25, 2016 08:09

JackStouffer reviewed Jan 11, 2017

View reviewed changes

dukc approved these changes Jan 21, 2017

View reviewed changes

wilzbach force-pushed the fix-15357-each-foreach branch from 7273b50 to f145959 Compare February 16, 2017 01:51

wilzbach force-pushed the fix-15357-each-foreach branch 2 times, most recently from 343b08b to e898090 Compare February 22, 2017 05:58

wilzbach mentioned this pull request Mar 12, 2017

Make each capable of early stopping #5268

Merged

wilzbach force-pushed the fix-15357-each-foreach branch 2 times, most recently from 867c068 to 9b5ce12 Compare May 7, 2017 11:37

dlang-bot added Review:Needs Work Merge:stalled labels Jun 22, 2017

dlang-bot added the Merge:Needs Rebase label Dec 29, 2017

wilzbach force-pushed the fix-15357-each-foreach branch from 9b5ce12 to 40e8db9 Compare February 7, 2018 00:25

wilzbach requested a review from andralex as a code owner February 7, 2018 00:25

wilzbach removed Merge:Needs Rebase Review:Needs Work Merge:stalled labels Feb 7, 2018

dlang deleted a comment from codecov-io Feb 7, 2018

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

wilzbach added the Merge:Blocked label Apr 3, 2018

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

wilzbach force-pushed the fix-15357-each-foreach branch from 40e8db9 to 3f44a4c Compare August 1, 2018 14:25

dlang-bot added the Severity:Enhancement label Aug 1, 2018

Fix issue 17019 - std.algorithm.iteration.each should be usable with …

3f44a4c

…parallel

andralex approved these changes Aug 14, 2018

View reviewed changes

andralex added Merge:auto-merge and removed Merge:Blocked labels Aug 14, 2018

dlang-bot merged commit bb769cf into dlang:master Aug 14, 2018

wilzbach deleted the fix-15357-each-foreach branch August 15, 2018 11:50

Uh oh!

Comments

Conversation

wilzbach commented Dec 23, 2016

Uh oh!

dlang-bot commented Dec 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Testing this PR locally

Uh oh!

PetarKirov 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!

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 Feb 16, 2017 • 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!

dukc left a comment

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Feb 16, 2017

Uh oh!

wilzbach commented Feb 28, 2017

Uh oh!

schveiguy commented Feb 28, 2017

Uh oh!

schveiguy commented Feb 28, 2017

Uh oh!

wilzbach commented Feb 7, 2018

Uh oh!

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

Uh oh!

Uh oh!

wilzbach commented Apr 3, 2018

Uh oh!

wilzbach commented Jun 6, 2018

Uh oh!

wilzbach commented Aug 14, 2018

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

dlang-bot commented Dec 23, 2016 •

edited

Loading

wilzbach Feb 16, 2017 •

edited

Loading

wilzbach commented Feb 7, 2018 •

edited

Loading