fix ClassCache#get(...) and ClassCache.CacheKey#hashCode()#1886
fix ClassCache#get(...) and ClassCache.CacheKey#hashCode()#1886gbrail merged 3 commits intomozilla:masterfrom
ClassCache#get(...) and ClassCache.CacheKey#hashCode()#1886Conversation
| int result = cls.hashCode(); | ||
| if (sec != null) { | ||
| result = sec.hashCode() * 31; | ||
| result = result * 31 + sec.hashCode(); |
There was a problem hiding this comment.
Do we have any knowledge / data about the "* 31" bit, which I assume was added long ago in history. It doesn't do anything to make this solution more correct -- if anything, it might make the hash table behave worse! Why don't we take it out and just add (or OR) the two values together?
There was a problem hiding this comment.
31 is a "magic number" used by many simple hash implementations, and I believe the * 31 part is just simulating Arrays.hashCode(...) behaviour, which will multiply the intermediate result by 31 before adding hashcode from the next array element.
In context here, yes, * 31 does look redundant, but I personally prefer XOR over add or logical add (aka OR), because I (somehow) believe it does better in mixing bits.
There was a problem hiding this comment.
Simple xoring for combining hashes isn’t great as two similar hashes will end up zeroing a lot of bits, and gives the same hash for A, B as for B, A. Multiplying by a small prime tends to spread a set of bits out over the available space and keeps this good for the mod N operation you’ll commonly end up using to choose a bucket in the table.
There are better / faster hashing functions out there, but they often need a final mixing steps so aren’t so well suited to hashCode() methods, and I don’t know how well they would combine with prime multiply and add hash codes.
|
@ZZZank thanks for discovering and fixing the hashcode problem. I probably implemented this wrong back then The good thing about this:
It is the default implementation in many cases. (JDK, autogenereated by many IDEs) |
| "top scope have no associated ClassCache and cannot have ClassCache associated due to not being a ScriptableObject"); | ||
| } | ||
| cache = new ClassCache(); | ||
| cache.associate(((ScriptableObject) topScope)); |
There was a problem hiding this comment.
@ZZZank Have you encountered a problem with the RuntimeException? Or have you simply found that the documentation doesn't match the implementation?
The fix seems to be not thread safe. If two threads tries to init the cache, you might have a race condition here.
At least in our application, we share one topLevelScope for multiple threads (and this scope gets a ClassCache assigned on construction)
Normally, when you use initStandardObjects on the top scope, a ClassCache should be added automatically. If you use "initSafeStandardObjects" (which means I don't want to use LiveConnect) it would be totally OK, if there is no classCache and I get the exception, because it means, that some code will try to access LiveConnect, that should not (and may be tries to break out the sandbox)
In my opinion, instead of implementing a possible non-threadsafe lazy init, the documentation should therefore be adapted, even if this is strictly speaking a change to the API.
BTW: The implementation was changed here: e59e568#diff-69ef3670c4b5abfaf14ea2f9571184d5193995fbea54dab635d3daff3f06c06b but the javadoc was not adjusted
There was a problem hiding this comment.
This problem was found when I was trying to use ClassCache#get(...) inside FunctionObject#<init>(...), in which case MozillaSuiteTest will fail. There're 4876 failed MozillaSuiteTest in total, the first of them is:
runMozillaTest[0, js=testsrc/tests/bigo-test.js, opt=false]
java.lang.RuntimeException: Can't find top level scope for ClassCache.get
at org.mozilla.javascript.ClassCache.get(ClassCache.java:84)
at org.mozilla.javascript.FunctionObject.<init>(FunctionObject.java:84)
at org.mozilla.javascript.ScriptableObject.defineFunctionProperties(ScriptableObject.java:2101)
at org.mozilla.javascript.drivers.ShellTest.runNoFork(ShellTest.java:434)
at org.mozilla.javascript.tests.MozillaSuiteTest.runMozillaTest(MozillaSuiteTest.java:197)
at java.base/java.lang.reflect.Method.invoke(Method.java:580)
|
OK, thanks -- I get the "+31 thing" in that context. Thanks for doing this! |
|
Anyway, I'm OK with the XOR for now seeing that we have a few different opinions and we can always test or adjust later if we need. |
4 participants
CacheKeywill completely ignore theclsfield whensecis present, now it will combine hash code for both fields.ClassCache.get(...)will now try to associate a new ClassCache object when none was found, then return the new object, as described in@returnjavadoc of it.Fixes #1884