Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement hash cache to fix #5280 #5289

Merged
merged 4 commits into from Aug 24, 2018
Merged

Implement hash cache to fix #5280 #5289

merged 4 commits into from Aug 24, 2018

Conversation

@ChrisBr
Copy link
Contributor

@ChrisBr ChrisBr commented Aug 23, 2018

This will fix #5280 as we only use eql? now on a bin and hash collision. Before we didn't check the hash as we did not store it. This will restore similar behavior before the new hash implementation.

ChrisBr added 4 commits Aug 23, 2018
To save memory we decided to not cache the hash value of the keys anymore
when using open addressing as this was only necessary for a fast bucket
skip when a bin collision happens. With the current approach, when a bin collision
happens we call eql? on the keys. This was reasonable as this only happens
when a collision occours.

However, with this approach we always call eql? when a bin collision happens even
when the hashes are different as we only use the lower bits of the hash
to calculate the bin where the key / value pair is stored. This causes now
that wrong implementation of eql? will crash always on a bin collisions and not
only on a bin AND hash collision.

As this is the case for bundler, there is no other way than caching the hash values
again to reduce the probability that this happens.

Fixes #5280 travis-ci/travis-ci#9994
We only need to store the amount of expected pairs in the hashes entry
and not the doubled size as we need in bins and entries.
because buckets is the actual size of the hash, so dividing is wrong
and would create a too small hash (which then needs to get resized).
@ChrisBr ChrisBr changed the title Implement hash cache to fix https://github.com/jruby/jruby/issues/5280 Implement hash cache to fix #5280 Aug 23, 2018
@ChrisBr
Copy link
Contributor Author

@ChrisBr ChrisBr commented Aug 23, 2018

Ok, CI looks quite good 👍

@kares kares added this to the JRuby 9.2.1.0 milestone Aug 24, 2018
@enebo enebo merged commit 067b279 into jruby:master Aug 24, 2018
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@eregon
Copy link
Member

@eregon eregon commented Aug 28, 2018

@ChrisBr Hello!

I beg to differ about the CI, because ruby/spec seem to get stuck after this change, see https://travis-ci.org/jruby/jruby/builds/419737258?utm_source=github_status&utm_medium=notification
I can also replicate locally with:

bin/jruby spec/mspec/bin/mspec run -fs -Gfails -Gcritical -Gslow spec/ruby/library/set

Reverting this PR with:

git revert -m 1 067b279320863654c3dc2af217a5d7165b4ef915

Allows those specs to pass.

@eregon eregon mentioned this pull request Aug 28, 2018
@ahorek
Copy link
Contributor

@ahorek ahorek commented Aug 28, 2018

I think #5278 should fix it, but there's a conflict @ChrisBr ?

@ChrisBr
Copy link
Contributor Author

@ChrisBr ChrisBr commented Aug 28, 2018

@eregon hope you had safe travels back from EuRuKo! Thanks for the report, I will look into it

@ChrisBr
Copy link
Contributor Author

@ChrisBr ChrisBr commented Aug 28, 2018

I think #5278 should fix it, but there's a conflict @ChrisBr ?

Thanks, I rebased #5278, let's see if this fixes the issue!

@ChrisBr
Copy link
Contributor Author

@ChrisBr ChrisBr commented Aug 28, 2018

@eregon I can not reproduce with
bin/jruby spec/mspec/bin/mspec run -fs -Gfails -Gcritical -Gslow spec/ruby/library/set

on current master...

But as this is related to Set, I could imagine that it might be related to

755ccdb

as if the hash gets initialized with a non pow of two size, the secondary hash function is not a generator and therefore could end in an infinite loop. It could never reproduce this but it could be theoretically possible.

Could you please try to reproduce with this PR: #5278 ?

@eregon
Copy link
Member

@eregon eregon commented Aug 28, 2018

@ChrisBr Odd, I can reproduce consistently on current master (067b279) with that command.

Using the latest commit of #5278 (6d44f80) seems to fix it, so let's merge that :)

Hope you had safe travels back too from EuRuKo :)

@ahorek
Copy link
Contributor

@ahorek ahorek commented Aug 28, 2018

probably the same problem on current master

gem install mime-types
irb
require 'mime-types'

-> hangs

#5278 (6d44f80) works

@eregon
Copy link
Member

@eregon eregon commented Aug 28, 2018

@ChrisBr I merged #5278, thanks for the quick fix :)

@ChrisBr
Copy link
Contributor Author

@ChrisBr ChrisBr commented Aug 28, 2018

@ahorek @eregon cool, thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants