ROR the callable_convert_cache key for better hash distribution

[nielsdos]

The bottom 3 bits are zeros due to alignment, to get better key distributions we should ROR the 3 bottom bits.
This results in a single extra instruction on x86 or arm64, which is negligible for the benefit we get.

[bwoebi]

You should rotate by the nearest lower power of two of min(sizeof(zend_internal_function), sizeof(zend_op_array)). I.e. ror by 7 instead of 3.
The reality is that user functions are going to be allocated ... aligned. I.e. they get put into the 256 byte bins in the allocator. So user funcs will all end up with the 8 lower bits zero. (internal funcs not necessarily because uses malloc, but the bits after that are unique.)

[nielsdos]

Right, good point! On 64 bit zend_internal_function is 160B and zend_op_array is 256B.
On 32-bit zend_internal_function is 92B large.
So on 64 bit it gets set in a 160B bin and on 32 bit in a 96B bin.
So on 64 bit we should use a ROR of 7 and on 32 bit a ROR of 6, AFAICT.

[nielsdos]

Actually, Bob's argument turns out not to be true: zend_internal_functions in EG(function_table) are allocated on the system heap. The system heap on my glibc system gives me an address with only the lowest 4 bits zeros because it adheres to an alignment of 16 bytes.

So I'm keeping this PR as-is.

[TimWolla]

If they are always 0, rotating them doesn't really do anything either. Just a shift should be sufficient.

weakrefs also use ZEND_MM_ALIGNMENT_LOG2:

php-src/Zend/zend_weakrefs.h

Lines 60 to 64 in bcb5612

	static zend_always_inline zend_ulong zend_object_to_weakref_key(const zend_object *object)
	{
	ZEND_ASSERT(((uintptr_t)object) % ZEND_MM_ALIGNMENT == 0);
	return ((uintptr_t) object) >> ZEND_MM_ALIGNMENT_LOG2;
	}

Should we have a generic function for pointer -> hash index?

[nielsdos]

The alignment guarantee is technically only 4 bytes for 32 bit on some platforms. So we can't throw away the bottom bits unconditionally. The weakmap case is fine as it's coming from ZendMM which has alignment guarantees we control.
I agree that we maybe should have some generic function for pointers in tables.
Worth noting that another trick that is sometimes used in PHP's code is to consider the pointer as a string of 4/8 bytes.

[bwoebi]

@nielsdos It's not a problem though, to throw these bits away (also we're not actually throwing them away, merely shifting to the end of the key). I mean, you're never going to have overlapping functions. The goal is to have a high entropy within the least significant bits, which you will still have for internal functions if you only consider bits 6/7 and upwards - only that if you don't, the entropy is low for userland functions.

[dstogov]

Rotated address is not significantly better. It's better to choose one of the well known hash function for integers e.g. from https://arxiv.org/pdf/1504.06804

[dktapps]

This is probably a problem in a lot of places, no? Why is a specialised solution needed in this particular instance?

[nielsdos]

This is probably a problem in a lot of places, no? Why is a specialised solution needed in this particular instance?

There are more places where we could do something like this. And in some places we already employ this (kind of) solution.

[iluuu1994]

The implementation will currently effectively only use one bucket, which is obviously bad. This looks like an improvement at least, though we may also just switch back to using the string name as a key for now. I'll leave that up to you.

[nielsdos]

I'll merge this already as an improvement as-is. It's not perfect, and not generic, but that can always be iterated on later. It's better than just using a single bucket (for low nr of entries). I'll put on my TODO to look at papers linked by Dmitry.

[dktapps]

This is also part of a wider spread issue that also affects integer-indexed arrays in userland too (e.g. arrays storing data in coordinate systems where the key is (X << 32) | Z). I guess integer key hashing should be explored in a more general context.

In any case, I for one won't complain about faster first-class callables 👍🏻

[iluuu1994]

I guess integer key hashing should be explored in a more general context.

Related: https://externals.io/message/96007