Responses inline. Best,-Matt On Tuesday, Aug 25, 2015 at 12:46 AM, Ken Thomases <ken(a)codeweavers.com>, wrote: Thanks for continuing to work on this. By no means a thorough review:
On Aug 24, 2015, at 10:49 PM, Matt Durgavich <mattdurgavich(a)gmail.com> wrote:
+#define CACHE_SIZE 256
+#define CACHE_DEPTH 16
+static SOCKET socket_cache[CACHE_SIZE][CACHE_DEPTH];
+
+/* Cache support */
+static void add_to_cache(SOCKET s);
+static BOOL remove_from_cache(SOCKET s);
+static BOOL socket_in_cache(SOCKET s);
I'm not sure this is a "cache", per se. It's a hash table, but naming issues are fairly minor. Let's go with socket_table
@@ -1470,8 +1479,24 @@ int WINAPI WSAStartup(WORD wVersionRequested, LPWSADATA lpWSAData)
INT WINAPI WSACleanup(void)
{
if (num_startup) {
- num_startup--;
- TRACE("pending cleanups: %d\n", num_startup);
+ /* WS_closesocket needs num_startup to be non-zero, so decrement afterwards */
+ if (num_startup - 1 == 0) {
Why not "if (num_startup == 1)"?
+static void add_to_cache(SOCKET s)
+{
+ int index, depth;
+ SOCKET old;
+ LONG *dest;
+ index = socket_to_index(s) % CACHE_SIZE;
If you always mod the result of socket_to_index() by CACHE_SIZE, why not do that in that function? Point taken.
+ for (depth = 0; depth < CACHE_DEPTH; ++depth) {
+ if (socket_cache[index][depth] == 0)
+ break;
+ }
+
+ if (depth == CACHE_DEPTH) {
+ ERR("Socket hash table collision\n");
This should exit here or you access beyond the end of socket_cache[index] just below. Good catch.
+ }
+
+ dest = (PLONG)&socket_cache[index][depth];
Don't use PLONG, just use LONG*. Ok
+ old = InterlockedExchange(dest, s);
+
+ if (old != 0) {
+ ERR("Socket hash table internal corruption");
Reporting corruption isn't right. You should use InterlockedCompareExchange() with 0 as the comparand to _avoid_ corruption. If it fails, loop back to the depth loop and try again. The only error possible should be filling the hash table bucket. Right. Makes sense. Will fix.
+ }
+}
+
+static BOOL remove_from_cache(SOCKET s)
+{
+ int index,depth;
+ SOCKET old;
+ LONG *dest;
+ index = socket_to_index(s) % CACHE_SIZE;
+ for (depth = 0; depth < CACHE_DEPTH; ++depth) {
+ if (socket_cache[index][depth] == s)
+ break;
+ }
+
+ if (depth == CACHE_DEPTH) {
+ return FALSE;
+ }
+
+ dest = (PLONG)&socket_cache[index][depth];
+ old = InterlockedExchange(dest, 0);
+ return (old == s);
Under what circumstances could old not equal s? If that can happen, then this should also use InterlockedCompareExchange(), otherwise you've removed some other socket. If I fork a child, I'll inherit their sockets right? So some paranoia in the remove is needed. Unless wine doesn't work that way. I'll write it as the analog to add. -Ken Thanks! I appreciate the feedback.