Skip to content

Conversation

@rvanvelzen
Copy link
Contributor

Right now the hash value for floats is their integer component. Instead, use dark magic to map the bits to an int.

Simple test script:

<?php
$map = new \Ds\Map();

for ($ii = 0, $max = 100000; $ii < $max; ++$ii) {
    $map[($ii / $max)] = $ii;
}

Time before patch: 16.721446990967
Time with patch: 0.012161016464233

Right now the hash value for floats is their integer component. Instead, use dark magic to map the bits to an int.
} hack;

hack.d = Z_DVAL_P(value);
return (uint32_t)(hack.ull ^ (hack.ull >> 32));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how exactly ds defines floating point equality for the purpose of HT lookups, but most likely 0. === -0., which would not be handled correctly by this hash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch - 0.0 and -0.0 are mapped to the same value on master.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikic === is used.

@rtheunissen
Copy link
Member

Looks good, if this passes we can merge. ✨

@rvanvelzen
Copy link
Contributor Author

(Closing and re-opening to bump Travis)

@rvanvelzen rvanvelzen closed this Oct 8, 2018
@rvanvelzen rvanvelzen reopened this Oct 8, 2018
@rtheunissen
Copy link
Member

Hah I restarted the build on Travis as well 😂

@rvanvelzen
Copy link
Contributor Author

It's still stuck 😞

@rtheunissen
Copy link
Member

Well this obviously isn't right.
https://travis-ci.org/php-ds/extension/jobs/438563025

@rtheunissen rtheunissen merged commit c38d1fe into php-ds:master Oct 8, 2018
@rtheunissen
Copy link
Member

I'll merge this now and sort the test suite out tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants