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

Use proper hash key collision detection scheme. #85

Merged
merged 4 commits into from
Jan 4, 2024

Conversation

ebarlas
Copy link
Contributor

@ebarlas ebarlas commented Jan 4, 2024

This is a replacement for PR #74.

This change is primarily about compliance. The prior version of my ebarlas submission didn't adequately handle theoretical hash key collisions. The current version now does that with an open addressing scheme.

Changes here also include various unrelated adjustments.

Implementation class: CalculateAverage_ebarlas

The exec time on an AWS EC2 c5.2xlarge with 8 vCPU is slightly higher than before, now clocking in at 10.1s. However, still noticeably lower than on the CX33 target env, interestingly.

real	0m10.192s
user	1m19.001s
sys	0m0.580s

This submission is still configured to run on GraalVM CE.

… concurrency from 16 to 8. Use bit mask rather than mod operator to confine hash code to table range.
@gunnarmorling
Copy link
Owner

@ebarlas, could you run this one:

./test.sh ebarlas

This is a little test suite I've added and there seem to be some failures. Thanks!

@ebarlas
Copy link
Contributor Author

ebarlas commented Jan 4, 2024

Yes! I'll take a look now.

@ebarlas
Copy link
Contributor Author

ebarlas commented Jan 4, 2024

Ah, the partitioning isn't working properly when it happens within a line! Getting that fixed now.

@ebarlas
Copy link
Contributor Author

ebarlas commented Jan 4, 2024

Fixed, @gunnarmorling!

@gunnarmorling
Copy link
Owner

Ah, gasp, just ran this one a minute ago. Do you plan to push further updates, @ebarlas?

@ebarlas
Copy link
Contributor Author

ebarlas commented Jan 4, 2024

Sorry about that! Not right at this moment.

@ebarlas
Copy link
Contributor Author

ebarlas commented Jan 4, 2024

It feels like we need a Discord server with all of the discussions happening.

@gunnarmorling
Copy link
Owner

Haha, yeah. Had I only known... ;)

@gunnarmorling
Copy link
Owner

0m14.476s. Nice!

@gunnarmorling
Copy link
Owner

The hashing scheme LGTM now too. Btw. thanks a lot for going through the iterative refinement of requirements today, appreciate the patience and quick turn around times to smoothen all that out.

@gunnarmorling gunnarmorling merged commit a8bd6b5 into gunnarmorling:main Jan 4, 2024
1 check passed
@ebarlas
Copy link
Contributor Author

ebarlas commented Jan 4, 2024

You're welcome! Needless to say, I've been having a lot of fun with this. So, thank you!

@ebarlas
Copy link
Contributor Author

ebarlas commented Jan 4, 2024

By the way, is 0m14.476s the official time? Just notice a higher figure on the leaderboard.

@ebarlas
Copy link
Contributor Author

ebarlas commented Jan 4, 2024

@gunnarmorling, is there anything additional I need to do to ensure the my solution is running with GraalVM?

I'm trying to understand the timing discrepancy I'm seeing and I thought perhaps my solution isn't actually running with GraalVM. Do I need an install command prior? I noticed the other competitors are apparently still relying on your manual config for GraalVM.

Relatedly, is the non-CE edition of Graal accessible?

sdk use java 21.0.1-graal

@gunnarmorling
Copy link
Owner

gunnarmorling commented Jan 4, 2024

By the way, is 0m14.476s the official time? Just notice a higher figure on the leaderboard.

Yeah, still had to update it.

@gunnarmorling, is there anything additional I need to do to ensure the my solution is running with GraalVM?

Not really, it's a manual (and thus, yes, error-prone) process. I'm quite confident I did it right. But if you'd like to explore how to automate this task (see #88), this would be awesome.

@gunnarmorling
Copy link
Owner

Probably just source "$HOME/.sdkman/bin/sdkman-init.sh" at the beginning of your launch script should do the trick.

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

Successfully merging this pull request may close these issues.

2 participants