Skip to content

Conversation

@fwbrasil
Copy link
Contributor

Problem

The hash function generation introduced by #895 has a few problems:

  • The function generation is slow and doesn't scale well when there are many hash switches
  • The cardinality of the functions is normally too high so the hash switch is very rarely selected
  • It often returns empty if the keys don't follow a pattern

Solution

I started optimizing the hash function generation and observed a pattern. The most successful functions were the ones that involved multiplication by a prime number. That led me to a simpler approach using just a prime factor and a shift.

The new hash function generation returns very fast and covers many more cases than the previous implementation. It can't handle well sets with larger cardinalities but that shouldn't be a problem since the function generation is fast and it would be discarded by LIRGenerator.emitStrategySwitch because of the low density of the table.

Notes

  • I removed the test code generation since it was based on the previous functions. I left the test cases because they cover scenarios that are still valid for the new implementation.
  • I fixed the JumpTable annotation to reflect the correct size of the entries
  • I'm working on a separate change to compile TypeSwitches using the hash table approach. I'm using a LongHasher very similar to the IntHasher and it also works well with memory addresses.

@fwbrasil
Copy link
Contributor Author

I've just fixed the formatting and unused imports. I'm not sure why LambdaStableNameTest is failing so I rebased my branch to see if that fixes it. All tests had passed locally yesterday.

@fwbrasil
Copy link
Contributor Author

LambdaStableNameTest is already failing on master: https://travis-ci.org/oracle/graal/jobs/652532656#L1384

@tkrodriguez
Copy link
Member

It seems to be a new test with some possibly overly strong assumptions. I've reported it but it seems like it might be specific to the JDK used in the travis gate as we don't see it in our internal gates, which use slightly different JDK versions. You could add an Ignore to that test so the gate can complete if you like but since I'll run it through our gate it shouldn't be necessary.

@fwbrasil
Copy link
Contributor Author

I've just fixed the copyright header and rebased. @erik1iu @tkrodriguez please let me know if there's anything missing, I wanted to create a PR that depends on this change. Thank you

@tkrodriguez
Copy link
Member

This looks ok to me. I'm running it through our gate now. It looks like checkstyle is unhappy with the final modifier on the static methods.

@e1iu
Copy link
Contributor

e1iu commented Mar 18, 2020

Looks good. BTW, You can use ecj to check code style locally:P

@fwbrasil
Copy link
Contributor Author

thanks for the quick replies. I've just removed the unnecessary static modifiers

@graalvmbot graalvmbot merged commit 161197a into oracle:master Mar 19, 2020
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.

5 participants