Fix Issue 18096 - Add fold() to std.parallelism #5951

acehreli · 2017-12-21T06:17:54Z

Implement TaskPool.fold similar to std.algorithm.fold: Seeds come after the range and need not be a tuple.

Plus a couple of link corrections in std.algorithm documentation.

dlang-bot · 2017-12-21T06:17:56Z

Thanks for your pull request, @acehreli! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close	Bugzilla	Description
✓	18096	Add fold() to std.parallelism

wilzbach

Congratulations on your first PR to Phobos, Ali!
We are trying to make Phobos code and documentation look modern, so there are a few things I commented on.

wilzbach · 2017-12-21T07:51:30Z

std/parallelism.d

+        ///
+        unittest
+        {
+            auto result = taskPool.fold!"a+b"([1, 2, 3, 4]);


Using string as a function isn't the recommended style anymore. Please use a real lambda function for the first example. So e.g.

fold!((a,b) => a +b)([1, 2, 3, 4]); ... fold!"a*b"([1, 2, 3, 4]);

This needs StdUnittest otherwise the unittest will be instantiated in user code. See the linked PR for details.

Then I'm inclined to fix TaskPool.reduce as well. Would it be appropriate in this branch?

As long as it's a separate commit, it's not a problem ;-)

Btw StdUnittest has been merged, so once you rebase to upstream/master, you can use it 😉

wilzbach · 2017-12-21T07:52:05Z

std/parallelism.d

+            auto result = taskPool.fold!("a+b", "a*b")([1, 2, 3, 4], 0, 1);
+            assert(result[0] == 10);
+            assert(result[1] == 24);
+        }


Needs StdUnittest

wilzbach · 2017-12-21T07:53:28Z

std/parallelism.d

+                assert(y[1] == 24);
+        }
+    }
+


You will need to add unittest here, so that your template is instantiated at least once and the unittest within it are run.

Thanks. And making sure the unittests run exposed the infamous

"Error: template instance fold!(function (a, b) => a + b) cannot use local '__funcliteral1' as parameter to non-global template fold(functions...)"

which means, I have to pull all unittests out to the module scope, which means, I can't take advantage of automatically inserting unittests as examples right under what they test. I think I will have to copy the code under Examples sections as had been done for TaskPool.reduce().

Any ideas for a solution?

The usual practice is to put the unittest block(s) right after the template declaration. This way the examples will still get inserted into the docs for the template. Which is fine in this case, because it's not a big deal to just leave the inner template with a blank ///, since the bulk of the docs will be on the outer template.

Furthermore, putting unittests inside a template is generally not a wise idea in terms of code bloat / possibly compilation time, because they will get instantiated for every instantiation of the template. Somebody tried to propose a static unittest sometime ago but it was shot down by Walter. Maybe we could try changing Walter's opinion on this again? :-P But in the meantime, keeping unittests in module scope seems to be the best compromise.

P.S. There's the workaround of using static if on a unittest inside a template so that it only gets instantiated for a specific template instantiation (i.e., only once), but that in turn requires something outside the template to trigger that specific instantiation in the first place, which makes it rather fragile.

Furthermore, putting unittests inside a template is generally not a wise idea in terms of code bloat / possibly compilation time, because they will get instantiated for every instantiation of the template.

FYI @quickfur we have StdUnittest for this purpose now (see #5927).

wilzbach · 2017-12-21T07:56:38Z

std/parallelism.d

+                          format("Invalid number of arguments (%s): Should be an input range, %s optional seed(s)," ~
+                                 " and an optional work unit size", Args.length, functions.length));
+            return fold(args[0], args[1..$]);
+        }


Yes that's the preferred Phobos style. It gives good error messages to people and doesn't hit them with a long list of overloads.
=> The other overloads should be private.

Alas, they disappear from documentation if they're made private.

Yes, that's the idea ;-)

But I want to keep the internal fold functions and their unittests in the documentations. (reduce has the similar template structure but it does not use nested unittests:

template fold(functions...) { // The outer template is almost useless as it's only for 'functions...'. The three different uses of Args... come next: auto fold(R)(R range) { return reduce!functions(range); } version (StdUnittest) unittest { // for this overload } //... etc }

So, that's why the nested ones cannot be private and I really want to keep the unittest right under their respective overloads.

wilzbach · 2017-12-21T07:59:21Z

std/parallelism.d

+
+            $(LREF fold) is functionally equivalent to $(LREF _reduce) except the
+            range parameter comes first and there is no need to use
+            $(REF_ALTTEXT $(D tuple),tuple,std,typecons) for multiple seeds.


Refs automatically use code boxes. If they don't we should fix it for all of them.
Also Phobos prefers backticks nowadays.

Thanks for the review. Yeah, the font was regular text font without $(D). I'll replace with backticks.

Thinking more about this, none of the _ALTTEXT macros should use code boxes as the alternative text can be any free form text. I've just checked dlang.org.ddoc and these macros look correct to me. So, back to backticks...

wilzbach · 2017-12-21T08:02:34Z

std/parallelism.d

+            The accumulated result as a single value for single function and as a tuple of values for multiple functions
+
+        See_Also:
+            $(LREF reduce)


std.algorithm.iteration.fold?

I really meant TaskPool.reduce here because std.algorithm.iteration.fold does not have the peculiarities of this module (mostly about the seeds).

Oh I meant: how about mentioning std.algorithm.iteration.fold too? (sorry for the brevity, I'm on my phone).
Btw I think it would be better to use TaskPool.reduce as LREF here, but from what I can see there's only MYREF, but no LREF_ALTTEXT yet :/

https://github.com/dlang/dlang.org/blob/master/std.ddoc

Also at least for Ddoc the link would be #.TaskPool.reduce, so maybe the ugly MYREF works already?

wilzbach · 2017-12-21T08:04:51Z

std/parallelism.d

    }

+    /** Implements the homonym function (also known as $(D accumulate), $(D
+        compress), $(D inject), or $(D foldl)) present in various programming


Nit: Phobos code prefers backticks, see e.g. #5801

wilzbach · 2017-12-21T08:05:34Z

std/parallelism.d

+
+        This is functionally equivalent to $(LREF reduce) except the range
+        parameter comes first and there is no need to use $(REF_ALTTEXT
+        $(D tuple),tuple,std,typecons) for multiple seeds.


Nit: backticks

acehreli · 2017-12-25T07:52:01Z

Here's one more iteration. To repeat myself, I like the inline unittest blocks as they give examples right under each overload.

Regarding string functions, they have a benefit over lambdas: We can't use lambdas with member function templates with the current implementation limitations (not enough context pointers). That's why I used static nested functions. I don't like it but I don't have a better option.

Thank you...

acehreli · 2017-12-25T08:12:22Z

Another problem with lambdas as opposed to string lambdas: dmd runs out of memory. :( (That's why the auto-tester failed. Oy!

acehreli · 2017-12-25T23:41:04Z

I had issues related to unittests. As one of the code comments explains, either the test was showing up at the wrong spot or it would cause dmd to run out of memory.

In the end, I had to do what reduce does: Hand-written Example section, which is unfortunate because these examples can't be edited or run by the users.

wilzbach

This is still missing a changelog entry and as all new public symbols it requires @andralex's approval.

wilzbach · 2017-12-26T00:42:35Z

std/parallelism.d

+            auto range()
+            {
+                return args[0];
+            }


Why not alias range = args[0];?

Sweet! I thought it would be rejected as args[0] is an expression.

wilzbach · 2017-12-26T00:45:01Z

std/parallelism.d

+                }
+                else
+                {
+                    static assert(0);


Needs an error message or simply use else // static if like e.g. here:

phobos/std/algorithm/iteration.d

Line 915 in 867ed5b

else // if (isRangeBinaryIterable!Range)

wilzbach · 2017-12-26T00:47:01Z

std/parallelism.d

+    // they would appear under the outer one. (We can't move this inside the
+    // outer fold() template because then dmd runs out of memory possibly due to
+    // recursive template instantiation, which is surprisingly not caught.)
+    version(StdUnittest)


version(StdUnittest) is only needed in templates - this isn't one anymore.

wilzbach · 2017-12-26T00:49:03Z

std/parallelism.d

+    version(StdUnittest)
+    @system unittest
+    {
+        static int adder(int a, int b)


Argh we should really fix 5710.

wilzbach · 2017-12-26T00:51:13Z

std/parallelism.d

+        // The range, the seed (0), and the work unit size (20)
+        auto z = taskPool.fold!adder([1, 2, 3, 4], 0, 20);
+        assert(z == 10);
+    }


Nit: usually we try to have an empty line before a new declaration.

wilzbach · 2017-12-26T01:01:21Z

std/parallelism.d

    }
 }

+version(StdUnittest)


Remove StdUnittest - it's as a workaround for the non-existing DIP82 only used for templates to avoid unittest block leakage into user code.

wilzbach · 2017-12-26T01:34:51Z

std/parallelism.d

+            static assert(args.length == 1 ||                         // just the range
+                          args.length == 1 + functions.length ||      // range and seeds
+                          args.length == 1 + functions.length + 1,    // range, seeds, and workUnitSize
+                          format("Invalid number of arguments (%s): Should be an input range, %s optional seed(s)," ~


Could be "%d" clarity. Also using std.format at CTFE is really expensive.

wilzbach · 2017-12-26T22:11:05Z

std/parallelism.d

+            // The range, the seed (0), and the work unit size (20)
+            auto z = taskPool.fold!adder([1, 2, 3, 4], 0, 20);
+            assert(z == 10);
+            ---


Obviously not a huge fan of this either, but fixing this is independent to this addition as reduce is equally affected...

…h template specializations

…tions, do not use format in CTFE, add more unit tests.

…age of dmd

acehreli · 2017-12-29T07:11:30Z

Once more... Hoping that the out-of-memory auto-test errors were due to proper lambdas. This commit uses string lambdas (only in non-documentation unittests).

vinobheeman · 2018-01-01T08:51:24Z

Hi All Approvers,

This pull request is still waiting for few of your approvals, so request your approval on the same.

From
Vino.B

acehreli · 2018-01-05T19:05:56Z

Is there anything else that I should do for this? Thanks...

quickfur · 2018-01-05T19:15:50Z

Adding new public symbols to Phobos requires @andralex's approval, so we need to somehow get his attention in order to move this forward.

wilzbach · 2018-01-16T00:35:03Z

Is there anything else that I should do for this? Thanks...

Yeah we really need to improve the status quo. Way too many @andralex PRs sink into the abyss :/

quickfur · 2018-01-16T00:38:00Z

I'm tempted, but haven't actually dared, to just merge it anyway and let @andralex push the Revert button afterwards if he doesn't approve. But that might be stepping over the line. :-P

quickfur · 2018-01-16T00:44:33Z

TBH, I think it's time to expand the @WalterBright + @andralex duo to include a few more trusted delegates that can make the less important decisions on their behalf. Requiring, e.g., @andralex's approval for every tiny public symbol added to Phobos, while understandable, is just not scalable. He gets burned out needing to review tons of relatively small changes (big-picture-wise speaking), and the rest of us get frustrated 'cos the queue is piling up with no end in sight. Lose-lose situation.

If he were to delegate, say, certain subsets of Phobos modules to certain people he trusts, that would help move things along at a more reasonable pace. To some extent this is already true for certain focused modules, like @jmdavis being the go-to person for std.datetime, or @DmitryOlshansky for std.uni, etc.. But Phobos is big, and we need more delegates to look after important parts like std.range, std.algorithm, just to name a few.

andralex · 2018-01-16T17:01:04Z

@quickfur already doing a lot of that... it's the swelling of PRs that's difficult to keep up, not the addition of names.

andralex · 2018-01-16T17:03:33Z

I'm tempted, but haven't actually dared, to just merge it anyway and let @andralex push the Revert button afterwards if he doesn't approve. But that might be stepping over the line. :-P

I wouldn't take offense with that, the only problem is it did happen to me in the past to leave it to others, then look over stuff after a while and see that matters took a definite turn for the worse - even as admitted by the authors! Please continue to keep me on the hook for new additions, thanks!

acehreli requested review from JackStouffer, PetarKirov, andralex and wilzbach as code owners December 21, 2017 06:17

dlang-bot added the Severity:Enhancement label Dec 21, 2017

acehreli force-pushed the TaskPool_fold branch from e61ebb6 to 6ca53fd Compare December 21, 2017 06:27

wilzbach reviewed Dec 21, 2017

View reviewed changes

wilzbach mentioned this pull request Dec 21, 2017

Add a StdUnittest version #5927

Merged

acehreli force-pushed the TaskPool_fold branch from 6ca53fd to 6f6cd1b Compare December 25, 2017 07:44

wilzbach reviewed Dec 26, 2017

View reviewed changes

wilzbach added the @andralex label Dec 26, 2017

acehreli added 6 commits December 26, 2017 22:40

Fix a couple of tuple links

0074d24

Fix Issue 18096 - Add fold() to std.parallelism

c472b23

Apply review comments: Backticks for inline code, StdUnittest, etc.

673c4e9

Implement std.parallelism.fold with static if branches instead of wit…

665e4ad

…h template specializations

Add the changelog entry

7519d93

Use message with static assert(0), use aliases instead of nested func…

8cfff5e

…tions, do not use format in CTFE, add more unit tests.

acehreli force-pushed the TaskPool_fold branch from 8ba2e34 to 8cfff5e Compare December 27, 2017 07:47

acehreli added 2 commits December 28, 2017 22:53

Merge branch 'master' of github.com:dlang/phobos into TaskPool_fold

fec204e

Use string lambdas in non-documentation unittests to reduce memory us…

f9e3dde

…age of dmd

remove surprising emtpy line

c1c8e86

andralex approved these changes Jan 16, 2018

View reviewed changes

andralex removed the @andralex label Jan 16, 2018

wilzbach added the Merge:auto-merge label Jan 17, 2018

dlang-bot merged commit fa0a619 into dlang:master Jan 17, 2018

Uh oh!

Fix Issue 18096 - Add fold() to std.parallelism #5951

Fix Issue 18096 - Add fold() to std.parallelism #5951

Uh oh!

Conversation

acehreli commented Dec 21, 2017

Uh oh!

dlang-bot commented Dec 21, 2017

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!

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!

acehreli commented Dec 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acehreli commented Dec 25, 2017

Uh oh!

acehreli commented Dec 25, 2017

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

acehreli commented Dec 25, 2017 •

edited

Loading