Fix issue 16705 - TaskPool.reduce fails to compile "cannot get frame pointer to D main"

[PetarKirov]

At the time std.parallelism was developed, module-level ddoc-ed unittests weren't available and probably that's why nobody noticed that example from the module synopsis broke.

[dlang-bot]

Fix	Bugzilla	Description
✓	16705	TaskPool.reduce fails to compile "cannot get frame pointer to D main"

[codecov-io]

Current coverage is 89.23% (diff: 100%)

Merging #4915 into stable will increase coverage by <.01%

@@             stable      #4915   diff @@
==========================================
  Files           121        121          
  Lines         76196      76203     +7   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          67989      68001    +12   
+ Misses         8207       8202     -5   
  Partials          0          0          

Powered by Codecov. Last update 9707f00...b44b768

[andralex]

Auto-merge toggled on

[andralex]

Seeing two timelimit errors, can you look into reducing the duration of unittests for std.parallelism?

[PetarKirov]

Sure, I will lower n to something like 1_000_000, though I would also need to update the timings.

[PetarKirov]

@andralex updated.

[yazd]

The timings are switched, right?

[PetarKirov]

The timings are for the large case. They are much lower, because I used LDC and a more powerful CPU.

[yazd]

So TaskPool.reduce is slower than the serial algorithm?

[PetarKirov]

Oh sorry, I didn't notice this before. Yes indeed the two timings should be swapped. I will submit a PR soon.

[PetarKirov]

Fixed in #4918.

[mihaipopescu]

Could you also comment on why the compiler generated "cannot get frame pointer to D main" ?

[PetarKirov]

@mihaipopescu the compiler error message is because since getTerm is local function, the compiler assumes that it has access to the local variables of main and would need to create closure in order to capture main's local state, so it can be available after main returns. (In this case the fact that no other user-defined function would be called after main is ignored by the compiler - main is treated like any other function.)
delegate function parameters* (i.e. pointers to functions that can access local state of other functions, or can bind to object methods) can be marked as scope telling the compiler that the captured local state would not out-live the function it comes from and in such situations the compiler does not need to allocate a closure on the heap. Since at least several years the D's compiler started to infer delegate parameters as scope in situations that it can prove that the captured state would not out-live the parent function's stack frame. This part of the compiler is constantly tweaked and improved in order to make users' live easier and since phobos is fairly well tested (most of the core modules have > 90% test coverage) such compiler regressions don't get a chance to sneak in, because each pull request to the compiler needs to pass the compiler, druntime and phobos test suites.
(@andralex please correct me if I'm wrong/missing something)

My change has two fixes in it:

• Change n and delta from run-time constants to compile-time constants - this makes it easier for the compiler to see that getTerm does not need to capture any state, out side of its scope.
• Change getTerm from an ordinary local function to a function template literal. Alternatively I could have changed to getTerm's signature to static real getTerm(int i), because static tells the compiler that a function should behave the same way as if it was declared at the module-level -> i.e. it won't capture local state.

At first glance this looks like a compiler regression, because the original code works with older compilers (up to DMD 2.068.2), but I need to investigate further, because it may be the case that TaskPool.reduce's code is at fault, since it wasn't well tested in the first place.

• alias and variadic template parameters can also receive delegates as it is the case with TaskPool.reduce.

[mihaipopescu]

Thank you @ZombineDev for the detailed explanation. I was actually able to understand and fix what was wrong in my own code. It would be very useful if the D reference compiler would have its errors standardised so users could understand what they are doing wrong.

[MartinNowak]

This shouldn't have said fix* in the commit mesage, b/c it just reference the Issue. You can see whether dlang-bot shows a ✓ or a ✕ in the fixes column.

[PetarKirov]

@MartinNowak agreed, but at first I thought that only the documentation needed to be fixed. After further investigation I came to the conclusion that there probably was a regression in the compiler, but at that time the PR had already been merged.