Eliminate static shared this from std/parallelism.d by andralex · Pull Request #5470 · dlang/phobos

andralex · 2017-06-11T18:41:44Z

Wondering how to get rid of the shared static ~this. cc @dsimcha @wilzbach @quickfur @JackStouffer @MartinNowak

dlang-bot · 2017-06-29T15:22:04Z

Thanks for your pull request, @andralex!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

JackStouffer · 2017-06-29T15:44:37Z

Maybe this is something I don't yet understand about D, but why the cast to @trusted pure instead of marking the impl function as @trusted pure?

andralex · 2017-06-29T16:03:57Z

Maybe this is something I don't yet understand about D, but why the cast to @trusted pure instead of marking the impl function as @trusted pure?

The cast doesn't only add the necessary attributes, it forces all attributes.

JackStouffer · 2017-06-29T18:10:40Z

I understand why we're getting rid of module constructors in general, but IMO this seems like one area where they are working as intended. Especially since the code is now more complex, and to get this work without constructors, an unsafe cast has to be used to lie to the user about the purity of the function.

@schveiguy I'm curious about your opinion on the purity question, as you had a lot of insight during the pure malloc discussion.

andralex · 2017-06-30T14:07:09Z

Updated the code with a reusable function that lazily initializes any atomically-loadable value. I plan to use that elsewhere and perhaps expose it publicly in the future.

@JackStouffer module constructors do a fine job but the standard library undergoes extra demands. It's expected of us to wander off the beaten path in the implementation.

andralex · 2017-08-11T19:12:28Z

cc @WalterBright @MartinNowak @CyberShadow need this reviewed because I can reuse it elsewhere - thx!

Burgos · 2017-08-12T15:55:58Z

std/parallelism.d

-        import core.sys.posix.unistd : _SC_NPROCESSORS_ONLN, sysconf;
-        totalCPUs = cast(uint) sysconf(_SC_NPROCESSORS_ONLN);
-    }
+    shared T result = outOfBandValue;


static shared?

PetarKirov · 2017-08-14T10:37:03Z

std/parallelism.d

+    auto local = atomicLoad(result);
+    if (local != outOfBandValue) return local;
+    // Long path
+    local = (cast(size_t function() @nogc nothrow pure @trusted) &initializer)();


You should check the type of initializer, because it may be delegate, a literal value, or any function pointer type pointing to a function taking one more arguments (which should be statically disallowed).

PetarKirov · 2017-08-14T10:41:32Z

std/parallelism.d

+{
+    return (cast(uint function() @nogc nothrow pure)
+        &totalCPUsImpl)();
+}


Can't you use lazilyInitializedConstant for totalCPUs?

ah, good idea brb

andralex · 2017-10-25T21:58:25Z

@ZombineDev updated

WalterBright · 2017-10-25T22:31:44Z

std/parallelism.d

+For that reason, no special precautions are taken so `initializer` may be called
+more than one time leading to benign races on the cached value.
+
+In the quiescent state the cost of the function is an atomic load from a global.


Need Params: and Returns: sections.

function is private, but will do

WalterBright · 2017-10-25T22:35:19Z

std/parallelism.d

+    {
+        static shared T result = outOfBandValue;
+        // Short path
+        auto local = atomicLoad(result);


should local be const ?

no, for generality

WalterBright · 2017-10-25T22:37:10Z

std/parallelism.d

+        auto local = atomicLoad(result);
+        if (local != outOfBandValue) return local;
+        // Long path
+        local = initializer();


Use different variable name here, as this use of local is completely independent of the previous one.

not getting it... it's the same var

Consider the trivial case:

int i = 4; foo(i); i = 5; // really the start of a different variable foo(i);

By the idea that each variable should have the tightest scope it can, when a variable has multiple disjoint lifetimes it should be split into multiple independent variables. It also often enables a variable to become const.

@WalterBright I understood the larger point you made with your first comment, but here local is and stays the local representation of the data meant to be cached throughout. We really are in good shape here.

WalterBright · 2017-10-25T22:39:54Z

std/parallelism.d

    import core.cpuid : datacache;
-    size_t lineSize = 0;
-    foreach (cachelevel; datacache)
+    foreach (ref cachelevel; datacache)


const ref ?

WalterBright · 2017-10-25T22:40:52Z

std/parallelism.d

+uint totalCPUsImpl() @nogc nothrow
+{
+    static shared uint result;
+    auto localResult = atomicLoad(result);


WalterBright · 2017-10-25T22:42:06Z

std/parallelism.d

+            auto nameStr = "hw.ncpu\0".ptr;
+        }
+
+        localResult = 0;


Independent use of localResult should not reuse the name.

it's in the same arc...

WalterBright · 2017-10-25T22:42:30Z

std/parallelism.d

+        }
+
+        localResult = 0;
+        size_t len = uint.sizeof;


Use const len instead of size_t len

doesn't work, see signature: https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man3/sysctlbyname.3.html

WalterBright · 2017-10-25T22:42:59Z

std/parallelism.d

 @property uint defaultPoolThreads() @trusted
 {
-    return atomicLoad(_defaultPoolThreads);
+    auto local = atomicLoad(_defaultPoolThreads);


WalterBright · 2017-10-25T22:43:55Z

As I recall, there was a correct way to do double checked locking using TLS variables. Perhaps this would be faster than going through atomicLoad().

John-Colvin · 2017-10-26T11:56:23Z

@WalterBright an easy way to use tls for the fast path

    static nothrow T impl()
    {
        // as Andrei wrote it, reading/writing to global shared ...
    }
    
    static nothrow tlsCache()
    {
        static T tls = outOfBandValue;
        if (tls == outOfBandValue)
            tls = impl();
        return tls;
    }

andralex · 2017-10-26T16:12:02Z

I updated the code to use a thread-local cache, starts to feel a bit overcooked. Anyhow it works nicely.

jmdavis · 2017-10-27T10:17:34Z

This PR broke dustmite

DustMite/dustmite.d(112): Error: function std.parallelism.lazilyInitializedConstant!(immutable(uint), 4294967295u, totalCPUsImpl).lazilyInitializedConstant is not accessible from module dustmite
../phobos/std/algorithm/iteration.d(1106):        instantiated from here: FilterResult!(__lambda2, string[])
DustMite/dustmite.d(106):        instantiated from here: filter!(string[])
DustMite/dustmite.d(163): Error: function std.parallelism.lazilyInitializedConstant!(immutable(uint), 4294967295u, totalCPUsImpl).lazilyInitializedConstant is not accessible from module dustmite

It looks like totalCPUs is now unusable outside of std.parallelism, because lazilyInitializedConstant is package, and aliases don't change the access level of what they alias.

PetarKirov · 2017-10-27T11:32:55Z

@jmdavis I propose to (temporary) revert this change in #5815.

…parallelism" This reverts commit 87bdeae, reversing changes made to b5e6365.

andralex · 2017-10-27T12:05:37Z

Oy what a bug that by definition can't be detected through unittesting :).

Fix visibility issue for #5470 merged-on-behalf-of: Petar Kirov <ZombineDev@users.noreply.github.com>

MartinNowak · 2017-10-27T21:50:46Z

That's the public alias to private symbol thing people mentioned recently. This will soon be fixed and should pass, atm. we still have the old (buggy) access checker running.
dlang/dmd#7241

andralex force-pushed the nomoresharedzisinparallelism branch 3 times, most recently from a4b2323 to 8bfa663 Compare June 11, 2017 19:04

andralex force-pushed the nomoresharedzisinparallelism branch from 8bfa663 to 7f2a8b0 Compare June 29, 2017 15:21

andralex force-pushed the nomoresharedzisinparallelism branch from 7f2a8b0 to d712d81 Compare June 30, 2017 13:59

andralex added the Review:Needs Review label Jun 30, 2017

Burgos reviewed Aug 12, 2017

View reviewed changes

PetarKirov reviewed Aug 14, 2017

View reviewed changes

Eliminate static shared this from std/parallelism.d

58c4f7d

andralex force-pushed the nomoresharedzisinparallelism branch from d712d81 to 9408826 Compare October 25, 2017 21:56

WalterBright reviewed Oct 25, 2017

View reviewed changes

andralex force-pushed the nomoresharedzisinparallelism branch 3 times, most recently from cf00ba7 to f0c986f Compare October 26, 2017 15:29

Review

286930a

andralex force-pushed the nomoresharedzisinparallelism branch from f0c986f to 286930a Compare October 26, 2017 16:10

Propagate safety properly

4f9b7ef

WalterBright approved these changes Oct 26, 2017

View reviewed changes

andralex added Merge:auto-merge and removed Review:Needs Review labels Oct 27, 2017

dlang-bot merged commit 87bdeae into dlang:master Oct 27, 2017

PetarKirov mentioned this pull request Oct 27, 2017

Revert "Eliminate static shared this from std/parallelism.d" #5814

Closed

PetarKirov added a commit to PetarKirov/phobos that referenced this pull request Oct 27, 2017

Revert "Merge pull request dlang#5470 from andralex/nomoresharedzisin…

a91371c

…parallelism" This reverts commit 87bdeae, reversing changes made to b5e6365.

PetarKirov mentioned this pull request Oct 27, 2017

Revert "Merge pull request #5470 #5815

Closed

dlang-bot added a commit that referenced this pull request Oct 27, 2017

Merge pull request #5816 from andralex/lazyInitializedConstant

27072f8

Fix visibility issue for #5470 merged-on-behalf-of: Petar Kirov <ZombineDev@users.noreply.github.com>

WalterBright mentioned this pull request Jan 27, 2023

std.socket: replace static this with crt_constructor #8670

Closed

Uh oh!

Comments

Conversation

andralex commented Jun 11, 2017

Uh oh!

dlang-bot commented Jun 29, 2017

Bugzilla references

Uh oh!

JackStouffer commented Jun 29, 2017

Uh oh!

andralex commented Jun 29, 2017

Uh oh!

JackStouffer commented Jun 29, 2017

Uh oh!

andralex commented Jun 30, 2017

Uh oh!

andralex commented Aug 11, 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!

andralex commented Oct 25, 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!

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!

WalterBright commented Oct 25, 2017

Uh oh!

John-Colvin commented Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andralex commented Oct 26, 2017

Uh oh!

jmdavis commented Oct 27, 2017

Uh oh!

PetarKirov commented Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andralex commented Oct 27, 2017

Uh oh!

MartinNowak commented Oct 27, 2017

Uh oh!

John-Colvin commented Oct 26, 2017 •

edited

Loading

PetarKirov commented Oct 27, 2017 •

edited

Loading