Eliminate static shared this in std.socket by andralex · Pull Request #5813 · dlang/phobos

andralex · 2017-10-26T21:31:47Z

A fresh approach based loosely on #5472.

MartinNowak · 2017-10-27T17:35:07Z

std/socket.d

+            auto ws2Lib = GetModuleHandleA("ws2_32.dll");
+            if (ws2Lib)
+            {
+                atomicStore(getnameinfoPointer, cast(typeof(getnameinfoPointer))


If I read this correctly, it's supported since WinXP.
https://msdn.microsoft.com/en-us/library/windows/desktop/ms738520(v=vs.85).aspx#SUPPORT_FOR_GETADDRINFO_ON_WINDOWS_2000_AND_OLDER_VERSIONS_

Yah, that's what @CyberShadow said too. Then we had linking problems so I backed off of that.

Just seeing those linking issues myself, it's an outed ws2_32.lib (OMF import lib for optlink) shipping with dmd, so much to that rotten format.

Yah, that's what @CyberShadow said too. Then we had linking problems so I backed off of that.

I just fixed those linking issues, dlang/installer#264, after 6 years :o, but it'll take a while till those libs arrive at the testers.

MartinNowak · 2017-10-27T17:42:55Z

std/socket.d

    {
-        WSACleanup();
+        static bool tlsInitialized;
+        if (tlsInitialized) return;


???
Why do you put a TLS flag around the double-checked locking in initOnce?

phobos/std/concurrency.d

Line 2487 in 27072f8

if (!atomicLoad!(MemoryOrder.acq)(flag))

Considering how slow name lookups are, performance shouldn't be a hindrance. And even then you'd better spent some time to optimize initOnce for dmd than to add more and more workarounds.

Just wanted to make initialize cheap to call in all instances.

It's already cheap to call initOnce, that's the purpose of it. I understand that dmd performs bad inlining with it, but that doesn't matter here.
And besides a TLS access isn't actually free either.

I'm a bit worried about cargo cult here.
That TLS locking pattern came up as alternative to "double-checked locking", only that double-checked locking works fine with atomic acquire&release primitives (which are free on X86).
So in total you have the overhead of a TLS access against a normal X86 load instruction.
Now in this PR you added both to be even faster.?

No problem, glad to remove.

dlang-bot · 2017-10-27T18:10:42Z

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.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#5813"

andralex · 2017-10-27T18:10:58Z

Fixed typo on Windows.

MartinNowak · 2017-10-28T01:34:39Z

std/socket.d

+            const val = WSAStartup(0x2020, &wd);
+            if (val)         // Request Winsock 2.2 for IPv6.
+                throw new SocketOSException("Unable to initialize socket library", val);
+            static extern(C) void cleanup() { WSACleanup(); }


Actually quite nice that dmd mangles nested C functions, no?

MartinNowak

The name resolution test is failing, complaining that WSA isn't initialized.

std.socket.AddressException@std\socket.d(1600): Unable to resolve host 'www.dlang.org': Either the application has not called WSAStartup, or WSAStartup failed.
----------------

I might just have a look since my Win VirtualBox is just on.

MartinNowak · 2017-10-28T01:38:12Z

std/socket.d

+            {
+                atomicStore(getnameinfoPointer, cast(typeof(getnameinfoPointer))
+                            GetProcAddress(ws2Lib, "getnameinfo"));
+                atomicStore(getaddrinfoPointer, cast(typeof(getaddrinfoPointer))


Those don't need to be atomic, since initOnce is holding a lock for you.

MartinNowak · 2017-10-28T01:39:19Z

std/socket.d

-        getaddrinfoPointer = &getaddrinfo;
-        freeaddrinfoPointer = &freeaddrinfo;
+        initialize;
+        return atomicLoad(getaddrinfoPointer)(a);


Those don't need to be atomic either since in case it really did initialize, the mutex has the necessary memory barriers.

MartinNowak · 2017-10-28T02:16:45Z

Looks like you still need to wrap gethostbyname and such functions not loaded from the DLL.

andralex · 2017-10-28T13:38:54Z

@MartinNowak thanks. I've made the updates prompted by your review save for calls to initialize. That'd be difficult to do without a Windows host. If you can get to this great, otherwise I'll return to this later.

quickfur · 2018-01-19T19:08:22Z

ping @andralex

Are we moving forward with this? Is it just a matter of a rebase?

andralex · 2018-01-19T20:16:57Z

@quickfur yes I'll get to this soon thanks for the prod

wilzbach · 2017-12-01T07:03:49Z

std/socket.d

-shared static ~this() @system nothrow @nogc
-{
-    version(Windows)
+    private void initialize() @trusted


CircleCi failure is due to the redundant usage of private:

same visibility attribute used as defined on line 329.

dejlek · 2019-02-04T12:42:12Z

So... where are we with this PR?

andralex · 2020-06-14T21:58:49Z

OK, I rebased. Let's see how it goes now.

andralex · 2020-06-14T22:02:21Z

std/socket.d

+            WSADATA wd;
+            const val = WSAStartup(0x2020, &wd);
+            if (val)         // Request Winsock 2.2 for IPv6.
+                throw new SocketOSException("Unable to initialize socket library", val);


I'm considering aborting the application here. It pretty much means the Windows installation is messed up. Thoughts?

Sounds like an horrible idea for GUI applications. Why?

There's just no realistic scenario that warrants throwing an exception (and forcing all transitive clients of this function to mind it). Last time WSAStartup() failed was on Windows 7 or whatever: https://www.youtube.com/watch?v=MWc6fGipU28.

Normally I wouldn't mind much, but it's time to acknowledge that throwing exceptions is painful for programmatic clients and we should only throw them when it makes sense. We need to tighten the screws on this stuff.

Oh, there is an alternative: simply return and leave the pointers to functions to be null. Then nothing will work.

OK, I think this is nice: if the lib doesn't load, no networking functionality is supported. There was code before that accounted for that anyway.

CyberShadow · 2020-06-15T04:18:42Z

std/socket.d

 }

+// Returns true iff getnameinfo is supported on this platform.
+version(Windows) private @property bool getnameinfoSupported()


Why add getaddrinfoSupported and getnameinfoSupported and not just make getnameInfoPointer / getaddrinfoPointer lazily-initialized property functions?

That's not new to this PR - old code handled the cases when the pointers were null and used workaround solutions.

So, old code:

try to fetch the pointers at startup

compare the pointers against null in code

New code:

define xxxSupported and xxx, both doing initialization on first call

use xxxSupported instead of comparing pointers to null

Right, but, why not just keep comparing pointers to null?

Because you need to call initialize before comparing them to null.

Yes, so, why not turn the function pointer variables into function pointer returning property functions, in the same way that getnameinfoSupported are in the current version, but returning the pointer directly instead of a bool?

That would be an alternative, but it seems more roundabout. To make sure I understand:

@property auto our_getnameinfoPtr() { initialize; return &getNameInfoPtr; } ... using code ... if (our_getnameinfoPtr) { ... our_getnameinfoPtr(... arguments ...); }

Is this the idea? Mine was to not use pointers at all, but instead have the simple "if xxxSupported returns true, you may call xxx" convention. It seems yours could be used to avoid two calls to initialize by using local variables:

... using code ... auto gni = our_getnameinfoPtr; if (gni) { ... gni(... arguments ...); }

That would be an alternative, but it seems more roundabout.

I don't see how that would be more roundabout. If anything, adding an additional pair of functions would better fit the description of "roundabout".

It seems yours could be used to avoid two calls to initialize by using local variables:

I don't see the need to avoid performing two calls. The compiler ought to inline the function, at which point it can trivially conclude that under the if branch, the pointer cannot possibly be null, and eliminate the second comparison.

Cool, yeah, two functions are better than four.

CyberShadow · 2020-06-15T04:20:41Z

std/socket.d

-shared static ~this() @system nothrow @nogc
-{
-    version (Windows)
+    void initialize() @trusted nothrow


If I'm reading this correctly, this function needs to be called in a lot more places now (every single bit of public code that eventually calls WSA functions, so that it calls WSAStartup), and I don't see this patch doing it.

Got it, will fix. Thanks.

andralex · 2020-06-18T01:57:02Z

Folks, I'll need to give up on this. There's too much work for Windows (albeit fun) but I don't have the time to set up Windows right now.

Eliminating static cdtors from druntime and phobos is key to lean and mean executables. Any function called from a static cdtors ends up linked in the program, transitively with all other functions it calls. And you know Phobos... you link one, you link'em all :)

andralex requested review from CyberShadow and dnadlinger as code owners October 26, 2017 21:31

andralex mentioned this pull request Oct 26, 2017

Eliminate shared this from std/socket.d #5472

Closed

MartinNowak reviewed Oct 27, 2017

View reviewed changes

andralex force-pushed the nomoresharedzisinsocketlastoneyay-redux branch from ac40755 to 0cc4fbc Compare October 27, 2017 18:10

MartinNowak reviewed Oct 28, 2017

View reviewed changes

andralex force-pushed the nomoresharedzisinsocketlastoneyay-redux branch from 2052ee0 to 50350e2 Compare October 28, 2017 13:37

dlang-bot added Merge:Needs Rebase and removed Merge:Needs Rebase labels Jan 1, 2018

wilzbach reviewed Apr 6, 2018

View reviewed changes

andralex mentioned this pull request Jun 14, 2020

Eliminate the need for shared static this() on Posix #7529

Closed

andralex added 3 commits June 14, 2020 17:57

Eliminate static shared this in std.socket

6e8be26

No more TLS

89238d2

One more typo on Windows

824bd7d

andralex force-pushed the nomoresharedzisinsocketlastoneyay-redux branch from 98218fb to 824bd7d Compare June 14, 2020 21:58

andralex commented Jun 14, 2020

View reviewed changes

Make initialize nothrow

f501da5

CyberShadow reviewed Jun 15, 2020

View reviewed changes

andralex closed this Jun 18, 2020

CyberShadow mentioned this pull request Jan 26, 2023

std.socket: replace static this with crt_constructor #8670

Closed

Uh oh!

Comments

Conversation

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

MartinNowak Oct 27, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Bugzilla references

Testing this PR locally

Uh oh!

andralex commented Oct 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

MartinNowak commented Oct 28, 2017

Uh oh!

andralex commented Oct 28, 2017

Uh oh!

quickfur commented Jan 19, 2018

Uh oh!

andralex commented Jan 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dejlek commented Feb 4, 2019

Uh oh!

andralex commented Jun 14, 2020

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

MartinNowak Oct 27, 2017 •

edited

Loading

dlang-bot commented Oct 27, 2017 •

edited

Loading