On Fri, Apr 5, 2019 at 9:26 AM Benjamin Coutu <ben.co...@zeyos.com> wrote:
> Hello,
>
> I was surprised to find out that equality checks on zend_string do not
> take advantage of the fact that in many cases we have a hash key available
> that can be utilized in a quick bailout path without having to resort to a
> costly `memcmp` on the value.
>
> I recommend to modify `zend_string_equal_content` like so:
>
> static zend_always_inline zend_bool zend_string_equal_content(zend_string
> *s1, zend_string *s2)
> {
> if (ZSTR_LEN(s1) != ZSTR_LEN(s2)) {
> return 0;
> }
>
> if (ZSTR_H(s1) && ZSTR_H(s1) != ZSTR_H(s2)) {
> return 0;
> }
>
> return zend_string_equal_val(s1, s2);
> }
>
> Thoughts?
>
We usually include the hash check explicitly in situations where it makes
sense, see in particular uses of zend_string_equal_content inside
zend_hash.c (such as
https://github.com/php/php-src/blob/ade73c360cbe9d533214e8b7cb26c514c29256fb/Zend/zend_hash.c#L614).
You'll note that these comparisons use the hash from the bucket rather than
the string, for better locality. Including a hash check in
zend_string_equal_content would result in a duplicate check there.
I think it might make sense to include a hash check inside
zend_string_equals() though, which is a higher-level function. From a quick
look, only one use of this function performs it's own hash check, which
could then be dropped.
Nikita
