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

Refactor and fix allocFirst with buckets for RubyHash #5278

Merged
merged 8 commits into from Aug 28, 2018

Conversation

Projects
None yet
5 participants
@ChrisBr
Copy link
Contributor

ChrisBr commented Aug 7, 2018

No description provided.

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Aug 7, 2018

@ChrisBr This all looked good at a glance but there are epic amounts of OOME in the travis build. Could that be related to these changes or is travis acting up?

@ChrisBr

This comment has been minimized.

Copy link
Contributor Author

ChrisBr commented Aug 8, 2018

This all looked good at a glance but there are epic amounts of OOME in the travis build. Could that be related to these changes or is travis acting up?

Likely 🤔 I will have a look at it tonight

@ChrisBr ChrisBr force-pushed the ChrisBr:refactoring/hash branch from 4b85450 to 8bcb10e Aug 9, 2018

@ChrisBr

This comment has been minimized.

Copy link
Contributor Author

ChrisBr commented Aug 9, 2018

Ok, I dropped a few suspicious commits, let's see if this solves it. I will after this PR is merged subsequently create new PRs with the dropped commits.

@ChrisBr ChrisBr force-pushed the ChrisBr:refactoring/hash branch from 8bcb10e to 49ce96e Aug 9, 2018

@kares kares added this to the JRuby 9.2.1.0 milestone Aug 21, 2018

@kares kares added the internal label Aug 21, 2018

ChrisBr added some commits Aug 7, 2018

Fix allocFirst with buckets
to use the next biggest power of two for the hash size. This is necessary because otherwise
the secondHashFunction might not be a generator and we do not iterate of all buckets.
It is a requirement for the secondaryHash function that buckets length is a power of two.
Extract lastElementsIndex function
to make updateStartAndEndPointer more readable.
Replace size == 0 with isEmpty function call
to make code more readable.
Simplify updateStartAndEndPointer function
it is not necessary to have an if/else we can just move the pointers up / down
until we find an existing key.

@ChrisBr ChrisBr force-pushed the ChrisBr:refactoring/hash branch from 49ce96e to 6d44f80 Aug 28, 2018

@eregon

eregon approved these changes Aug 28, 2018

Copy link
Member

eregon left a comment

Fixes #5289 so seems good to me :)

Arrays.fill(bins, EMPTY_BIN);
}
}

private final int nextPowOfTwo(final int i) {

This comment has been minimized.

Copy link
@ahorek

ahorek Aug 28, 2018

Contributor

for inspiration jemalloc/jemalloc#1303

@eregon

This comment has been minimized.

Copy link
Member

eregon commented Aug 28, 2018

Let's merge this, CI is green (except just one failure of ruby/spec which I will tag).

@eregon eregon merged commit 0aef1ec into jruby:master Aug 28, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.