std.parallelism fatal flaw? by burner · Pull Request #4399 · dlang/phobos

burner · 2016-06-03T09:12:50Z

Big parts of the std.parallelism unittests where not run for a very long time.
Apparently they do not even compile anymore. This PR refactors the unittests
so they are at least run once by default. While refactoring I found a couple of
problems with the unittests. Such as, local unittest function being unable to
access the current stackframe. This destroyed some constructs used by
std.parallelism.

I'm a noob when it comes to std.parallelism. I found this bug while scoping
imports. I need help on this!

burner · 2016-06-03T09:26:12Z

std/parallelism.d

-                    foreach (i; 0..1_000_000) {}
-                }
+    // Get around weird bugs having to do w/ sqrt being an intrinsic:
+    //real[][] sqrtMatrix = poolInstance.amap!parallelSqrt(matrix);


amap can not call parallelSqrt that that local function was trying to access poolInstance which is located on the same stack

burner · 2016-06-03T09:40:56Z

std/parallelism.d

-                               )
-                           );
+    // Get around weird bugs having to do w/ sqrt being an intrinsic:
+    real[][] sqrtMatrix = poolInstance.amap!parallelSqrt(matrix);


amap can not call parallelSqrt that that local function was trying to access poolInstance which is located on the same stack

joakim-noah · 2016-06-05T02:45:12Z

Well, it is a stress test, but yeah, probably nobody was running it and it's better to have it run once like this by the tester, so it's kept up to date.

wilzbach · 2016-06-05T10:11:06Z

std/parallelism.d

-            assert(poolInstance.workerIndex() == 0);
+// These unittests are intended more for actual testing and not so much
+// as examples.
+void stressTest2(int poolSize)


Huh - what was the reason to expose such test methods publicly?
Shouldn't they be private and have version(unittest)?

I created this function to have runnable one or multiple times. Eventually, this will be private. But this is not the problem here. Please keep the nitpicking to a minimum for now.

burner · 2016-06-16T13:07:52Z

@joakim-noah not only that but the design concept used by the test is no longer supported by D. Which begs the question if std.parallelism is still a working phobos module or if it needs to be deprecated.

andralex · 2016-06-18T08:04:11Z

std/parallelism.d

+{
+    static import std.math;
+    return std.math.isNaN(f);
+}


@burner shouldn't these be one import?

I thought so to, but isNaN was passed as a function to a function of parallelism. But this code has not been run at least since isNaN became a template. This is just code trying to get the tests into a compileable state.

JackStouffer · 2016-06-27T22:42:58Z

std/parallelism.d

-            // matrix.
+    static real[] parallelSqrt(real[] nums)
+    {
+        return poolInstance.amap!(mySqrt)(nums);


Your first problem is that static functions aren't allowed to access poolInstance

dnadlinger · 2016-06-27T23:16:28Z

Before being quite so fatalistic and going around suggesting modules to be deprecated, you might want to make sure which of the issues actually existed before you touched the code. isNaN having become a template is taken into account easily enough, and after that, enabling the test by default seems to work without any further issues (auto-tester still pending, see #4484 for test results).

burner · 2016-06-28T09:13:37Z

@klickverbot I recreated your PR here. The difference is that I moved the two unittests into a function that takes the number of runs as an argument. Now I get:

std/parallelism.d(4467): Error: static function std.parallelism.testFunction2.parallelSqrt cannot access frame of function std.parallelism.testFunction2
std/parallelism.d(4495): Error: static function std.parallelism.testFunction2.parallelSum cannot access frame of function std.parallelism.testFunction2

I really could need some help. I do not see the problem.

JackStouffer · 2016-06-28T13:11:28Z

std/parallelism.d(4467): Error: static function std.parallelism.testFunction2.parallelSqrt cannot access frame of function std.parallelism.testFunction2
std/parallelism.d(4495): Error: static function std.parallelism.testFunction2.parallelSum cannot access frame of function std.parallelism.testFunction2

I really could need some help. I do not see the problem.

As I said in my earlier comment, it means exactly what it says: you're trying to get a variable that belongs to testFunction2 inside of a static function. Specifically, you're trying to access poolInstance.

burner · 2016-06-29T08:35:52Z

std/parallelism.d(4467): Error: template instance amap!(mySqrt) cannot use local 'mySqrt' as parameter to non-global template amap(functions...)
std/parallelism.d(4470): Error: template instance amap!(parallelSqrt) cannot use local 'parallelSqrt' as parameter to non-global template amap(functions...)
std/parallelism.d(4499): Error: template instance amap!(parallelSum) cannot use local 'parallelSum' as parameter to non-global template amap(functions...)

IMO the problem is that unittest block statements are somehow different from function block statements. And the test/use-case of std.parallelism used that fact.

dnadlinger · 2016-06-29T09:54:35Z

If you keep it using the global poolInstance and the Sqrt/Sum functions static, it should work.

JackStouffer · 2016-07-14T13:35:02Z

Ping @burner

burner · 2016-07-14T14:24:43Z

I hadn't had much time lately. But its on my list

wilzbach · 2017-02-26T19:26:11Z

Hmm we should really enable the test suite. There's some random (un)-coverage business going on in at least these places of std.parallelism:

636:    throw exception;

https://codecov.io/gh/dlang/phobos/compare/86050a1c59eda327fa263549f0b53e56a4cfa9ed...2749175ceb24e4b780725677b7e233f65e4f791c/changes

3755 assert(lastException);
3756 lastException.next = e;
3757 lastException = findLastException(e);

https://codecov.io/gh/dlang/phobos/compare/77f1a9067b59395f8444cb2d01047680aebcfe5f...2e05f29d3c4cc7690198c3ad0d06eb23293da48a/changes

burner mentioned this pull request Jun 3, 2016

std.parallelism with selective scoped imports #4391

Closed

burner reviewed Jun 3, 2016
View reviewed changes

burner force-pushed the parallelism_fatal_flaw branch from 322ddd1 to 67e0411 Compare June 3, 2016 09:40

burner reviewed Jun 3, 2016
View reviewed changes

wilzbach reviewed Jun 5, 2016
View reviewed changes

burner mentioned this pull request Jun 14, 2016

Issue 16142: Correct test for hasElaborateAssign #4421

Merged

andralex reviewed Jun 18, 2016
View reviewed changes

burner mentioned this pull request Jun 27, 2016

Removed global imports in std.parallelism #4481

Closed

JackStouffer reviewed Jun 27, 2016
View reviewed changes

JackStouffer mentioned this pull request Jun 27, 2016

parallelism: Always run one round of the stress test #4484

Closed

minimal change

b60c33b

burner force-pushed the parallelism_fatal_flaw branch from 67e0411 to b60c33b Compare June 28, 2016 09:00

9il added the Review:Needs Work label Jul 29, 2016

wilzbach mentioned this pull request Feb 26, 2017

Derandomize std.algorithm #5202

Merged

burner closed this May 2, 2017

Uh oh!

Conversation

burner commented Jun 3, 2016

Uh oh!

burner Jun 3, 2016

Choose a reason for hiding this comment

Uh oh!

burner Jun 3, 2016

Choose a reason for hiding this comment

Uh oh!

joakim-noah commented Jun 5, 2016

Uh oh!

wilzbach Jun 5, 2016

Choose a reason for hiding this comment

Uh oh!

burner Jun 5, 2016

Choose a reason for hiding this comment

Uh oh!

burner commented Jun 16, 2016

Uh oh!

andralex Jun 18, 2016

Choose a reason for hiding this comment

Uh oh!

burner Jun 18, 2016

Choose a reason for hiding this comment

Uh oh!

JackStouffer Jun 27, 2016 • 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!

dnadlinger commented Jun 27, 2016

Uh oh!

burner commented Jun 28, 2016

Uh oh!

JackStouffer commented Jun 28, 2016

Uh oh!

burner commented Jun 29, 2016

Uh oh!

dnadlinger commented Jun 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JackStouffer commented Jul 14, 2016

Uh oh!

burner commented Jul 14, 2016

Uh oh!

wilzbach commented Feb 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

JackStouffer Jun 27, 2016 •

edited

Loading

dnadlinger commented Jun 29, 2016 •

edited

Loading