Re: [APC-DEV] Re: fix inode/device usage for win32, take #1

Hey Pierre,

Pierre Joye wrote:
> On Sun, Oct 18, 2009 at 8:57 PM, Pierre Joye<[email protected]>  wrote:
>> hi,
>>
>> The patch seems to be incomplete. The test case from the bug report
>> fails with apc.stat=0, on the 2nd request, with:
>
>
> Wrong alert, it was a configuration issue on one of the test VM
> (include path was wrong and cwd got this file too...). The patch
> passes our tests now.
>

Cool, thanks for the TS fixes btw.


> It is however a relative big change for a bug fix release. I can split
> the patch in two different changes:
>
> - the inode/device ifx
> - the zend handle addition + fstat usage
>

Sorry I was out of town so catching up on this thread... Yay some more windows fixes :-D

-----------
  /* {{{ hash */
+#ifdef PHP_WIN32
+static __int64 hash(apc_cache_key_t key)
+{
+    return key.data.file.device + key.data.file.inode;
+}
+#else
  static unsigned int hash(apc_cache_key_t key)
  {
      return key.data.file.device + key.data.file.inode;
  }
+#endif
  /* }}} */
-----------
Is the __int64 necessary here, I think we just do a "hash(key) % cache->num_slots" with this in which case I think we should up the precision of this operation if we think it will help.

On another related note, perhaps not for this fix though, this function seems unnecessary to me at this point, I think we should probably make it into something that actually generates a more useful hash or just manually inline the addition function.  Thoughts?  As a todo item I'd like to integrate some stats on our bucket counts to see how well the hashing is working for all keys.

Would it be of use for us to make some generic API functions to handle our OS specific code here, I assume we may get into some more #ifdef blocks as we fixup some more windows odds and ends?  I don't want to go to too much trouble, but if everyone thinks that'll help might be worth starting now?


> I could also help to have a last RC, what do you think?

I think this is mostly windows related, and so I assume will be of more help than harm so I'd vote for it going into a stable release as a whole, but I'll defer to Gopal if he has a stability concern.  I'm +1 on an RC in that case.



-shire