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

hi!

On Wed, Oct 21, 2009 at 7:55 PM, shire <[email protected]> wrote:
>> 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
>
> -----------
> =A0/* {{{ hash */
> +#ifdef PHP_WIN32
> +static __int64 hash(apc_cache_key_t key)
> +{
> + =A0 =A0return key.data.file.device + key.data.file.inode;
> +}
> +#else
> =A0static unsigned int hash(apc_cache_key_t key)
> =A0{
> =A0 =A0 return key.data.file.device + key.data.file.inode;
> =A0}
> +#endif
> =A0/* }}} */
> -----------
> 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 precis=
ion
> of this operation if we think it will help.

The volume id fits in 32bit, the file id is created from two DWORD
(32bit int), the low and high parts. That's why we need 64bit integer
here (unsigned even, I will fix that before applying the patch).

> 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 manuall=
y
> inline the addition function. =A0Thoughts? =A0As 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 ou=
r
> OS specific code here, I assume we may get into some more #ifdef blocks a=
s
> we fixup some more windows odds and ends? =A0I don't want to go to too mu=
ch
> trouble, but if everyone thinks that'll help might be worth starting now?

Yes, that definitively makes more sense and it is cleaner. However it
will introduce even more changes in the 3.0 branche. I would like to
do that in 4.x instead.

>> 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 h=
elp
> 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. =A0I'm +1 on an RC in =
that
> case.

The last point we discussed with Gopal was the zend file handler. It
is actually a little optimisation. In some cases the engine already
has a stream openened for a given path or file. By passing it to APC
we can use fstat or use the handle directly for
GetFileInformationByHandle (stat-like for win). I can remove this part
and only add the ino_t/dev_t changes to 3.0 while adding them to 4.0
(with some cleaner wrappers). Comments?

Cheers,
--=20
Pierre

http://blog.thepimp.net | http://www.libgd.org