GitHub PullRequestReviewCommentEvent at 2014-01-25 21:56:07
Repository: phobos
Actor: blackwhale
Payload: {"comment":{"url":"https://api.github.com/repos/D-Programming-Language/phobos/pulls/comments/9173170","id":9173170,"diff_hunk":"@@ -2001,109 +2001,162 @@ deprecated(\"Please use std.socket.TimeVal instead.\") alias TimeVal timeval;\n /**\n  * A collection of sockets for use with $(D Socket.select).\n  *\n- * $(D SocketSet) allows specifying the capacity of the underlying\n- * $(D fd_set), however users should be aware that the exact meaning of this\n- * value varies depending on the current platform:\n- * $(UL $(LI On POSIX, $(D fd_set) is a bit array of file descriptors. The\n- * $(D SocketSet) capacity specifies the highest file descriptor which can be\n- * stored in the set.)\n- * $(LI on Windows, $(D fd_set) is an array of socket handles. Capacity\n- * indicates the actual number of sockets that can be stored in the set.))\n+ * $(D SocketSet) wraps the platform $(D fd_set) type. However, unlike\n+ * $(D fd_set), $(D SocketSet) is not statically limited to $(D FD_SETSIZE)\n+ * or any other limit, and grows as needed.\n  */\n class SocketSet\n {\n private:\n-    version(Windows)\n+    version (Windows)\n     {\n-        // the maximum number of sockets the allocated fd_set can hold\n-        uint fdsetCapacity;\n+        // On Windows, fd_set is an array of socket handles,\n+        // following a word containing the fd_set instance size.\n+        // We use one dynamic array for everything, and use its first\n+        // element(s) for the count.\n+\n+        alias typeof(fd_set.init.fd_count) fd_set_count_type;\n+        alias typeof(fd_set.init.fd_array[0]) fd_set_type;\n+        static assert(fd_set_type.sizeof == socket_t.sizeof);\n+\n+        // Number of fd_set_type elements at the start of our array that are\n+        // used for the socket count and alignment\n+\n+        enum FD_SET_OFFSET = fd_set.fd_array.offsetof / fd_set_type.sizeof;\n+        static assert(FD_SET_OFFSET);\n+        static assert(fd_set.fd_count.offsetof % fd_set_type.sizeof == 0);\n+\n+        fd_set_type[] set;\n+\n+        final void resize(size_t size)\n+        {\n+            set.length = FD_SET_OFFSET + size;\n+        }\n+\n+        final ref fd_set_count_type count() @property inout\n+        {\n+            assert(set.length);\n+            return *cast(fd_set_count_type*)set.ptr;\n+        }\n \n-        fd_set* set;\n-        @property uint count() const { return set.fd_count; }\n+        final size_t capacity() @property const\n+        {\n+            return set.length - FD_SET_OFFSET;\n+        }\n+\n+        final inout socket_t[] fds() inout @property\n+        {\n+            return cast(socket_t[])set[FD_SET_OFFSET..FD_SET_OFFSET+count];\n+        }\n     }\n-    else version(Posix)\n+    else\n+    version (Posix)\n     {\n-        int fdsetMax;\n+        // On Posix, fd_set is a bit array. We assume that the fd_set\n+        // type (declared in core.sys.posix.sys.select) is a structure\n+        // containing a single field, a static array.\n \n-        fd_set setData;\n-        final @property fd_set* set() { return &setData; }\n-        final @property const(fd_set)* set() const { return &setData; }\n-        int maxfd;\n-        uint count;\n-    }\n+        static assert(fd_set.tupleof.length==1);\n \n+        // This is the type used in the fd_set array.\n+        // Using the type of the correct size is important for big-endian\n+        // architectures.\n \n-public:\n+        alias typeof(fd_set.init.tupleof[0][0]) fd_set_type;\n \n-    /**\n-     * Set the capacity of this $(D SocketSet). The exact meaning of the\n-     * $(D max) parameter varies from platform to platform.\n-     * Throws: $(D SocketParameterException) if $(D max) exceeds this\n-     * platform's maximum socket set size.\n-     */\n-    this(uint max)\n-    {\n-        version(Windows)\n+        // Number of file descriptors represented by one fd_set_type\n+\n+        enum FD_NFDBITS = 8 * fd_set_type.sizeof;\n+\n+        static fd_set_type mask(uint n)\n         {\n-            fdsetCapacity = max;\n-            set = FD_CREATE(max);\n+            return (cast(fd_set_type)1) << (n % FD_NFDBITS);\n         }\n-        else version(Posix)\n+\n+        // Array size to fit that many sockets\n+\n+        static size_t lengthFor(size_t size)\n         {\n-            // TODO (needs druntime changes)\n-            enforce(max <= FD_SETSIZE, new SocketParameterException(\n-                \"Maximum socket set size exceeded for this platform\"));\n-            fdsetMax = max;\n+            return (size + (FD_NFDBITS-1)) / FD_NFDBITS;\n         }\n-        reset();\n+\n+        fd_set_type[] set;\n+\n+        final void resize(size_t size)\n+        {\n+            set.length = lengthFor(size);\n+        }\n+\n+        // Make sure we can fit that many sockets\n+\n+        final void setMinCapacity(size_t size)\n+        {\n+            auto length = lengthFor(size);\n+            if (set.length < length)\n+                set.length = length;\n+        }\n+\n+        final size_t capacity() @property const\n+        {\n+            return set.length / FD_NFDBITS;\n+        }\n+\n+        int maxfd;\n     }\n+    else\n+        static assert(false, \"Unknown platform\");\n \n-    /// Uses the default capacity for the system.\n-    this()\n+public:\n+\n+    /**\n+     * Create a SocketSet with a specific initial capacity (defaults to\n+     * $(D FD_SETSIZE), the system's default capacity).\n+     */\n+    this(size_t size = FD_SETSIZE)\n     {\n-        this(FD_SETSIZE);\n+        resize(size);\n+        reset();\n     }\n \n     /// Reset the $(D SocketSet) so that there are 0 $(D Socket)s in the collection.\n     void reset()\n     {\n-        FD_ZERO(set);\n-\n-        version(Posix)\n+        version (Windows)\n+            count = 0;\n+        else\n         {\n+            set[] = 0;\n             maxfd = -1;\n-            count = 0;\n         }\n     }\n \n \n     void add(socket_t s)\n     {\n-        // Make sure too many sockets don't get added.\n-        version(Windows)\n-        {\n-            enforce(count < fdsetCapacity, new SocketParameterException(\n-                \"SocketSet capacity exceeded\"));\n-        }\n-        else version(Posix)\n+        version (Windows)\n         {\n-            enforce(s < fdsetMax, new SocketParameterException(\n-                \"Socket descriptor index exceeds SocketSet capacity\"));\n+            if (count == capacity)\n+                set.length *= 2;","path":"std/socket.d","position":194,"original_position":194,"commit_id":"4c8bc72069bac132c16c6a22ebc9214b0f3970bf","original_commit_id":"4c8bc72069bac132c16c6a22ebc9214b0f3970bf","user":{"login":"blackwhale","id":281770,"avatar_url":"https://gravatar.com/avatar/16611904eabb61195a73f8444976979f?d=https%3A%2F%2Fidenticons.github.com%2Fd541c7f4e9b0b3207cfbe90e0c0fca1c.png&r=x","gravatar_id":"16611904eabb61195a73f8444976979f","url":"https://api.github.com/users/blackwhale","html_url":"https://github.com/blackwhale","followers_url":"https://api.github.com/users/blackwhale/followers","following_url":"https://api.github.com/users/blackwhale/following{/other_user}","gists_url":"https://api.github.com/users/blackwhale/gists{/gist_id}","starred_url":"https://api.github.com/users/blackwhale/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/blackwhale/subscriptions","organizations_url":"https://api.github.com/users/blackwhale/orgs","repos_url":"https://api.github.com/users/blackwhale/repos","events_url":"https://api.github.com/users/blackwhale/events{/privacy}","received_events_url":"https://api.github.com/users/blackwhale/received_events","type":"User","site_admin":false},"body":"AFAICT array concat does maintain a cache for capacity of sorts that avoids the need to lock the GC on every append if there is enough space in the block.\r\nSee https://github.com/D-Programming-Language/druntime/blob/master/src/rt/lifetime.d#L446\r\nThis was called array append cache long time ago when it was all news.\r\nPinging @schveiguy as the author of it.\r\n\r\nI have 2 suggestions:\r\n1. Keep *= 2 but extend the array to its true capacity, i.e. \r\n`capacity = set.length = set.capacity;`\r\nNot exactly the definition of cool but will avoid wasting memory.\r\n\r\n2. Just use ~= and let the built-in reallocation amortizing actually work. ","created_at":"2014-01-25T21:56:07Z","updated_at":"2014-01-25T21:56:07Z","html_url":"https://github.com/D-Programming-Language/phobos/pull/1393#discussion_r9173170","pull_request_url":"https://api.github.com/repos/D-Programming-Language/phobos/pulls/1393","_links":{"self":{"href":"https://api.github.com/repos/D-Programming-Language/phobos/pulls/comments/9173170"},"html":{"href":"https://github.com/D-Programming-Language/phobos/pull/1393#discussion_r9173170"},"pull_request":{"href":"https://api.github.com/repos/D-Programming-Language/phobos/pulls/1393"}}}}