Add support for constant_dynamic with SymbolKey as an example. - #2461
aardvark179 wants to merge 8 commits into
Conversation
8ee5ea8 to
770d922
Compare
|
I've tried adding regexps to the literals we can handle, and the results look pretty positive. In the V8 regexp benchmark we see a good reduction in recompilations. Before: and after: Not quite at the level of benchmark that measures anything very useful, yet, but at least we're reducing the time and variance caused by the JIT thrashing away. I tried looking at whether we could pull the same sort of thing for |
|
This is very interesting. I think we should merge this just because of the improvement it makes to regexp stuff. I'm not sure where else it helps since so many JavaScript constants are for things like big precomputed objects but I'm certainly willing to find out. Any reason we shouldn't just do this? |
aardvark179
changed the title
SymbolKey as an example.SymbolKey as an example.
a93ae5f to
5341889
Compare
gbrail
left a comment
gbrail
left a comment
There was a problem hiding this comment.
My AI very diligently churned on this for a long time but there is a real bug here.
It turns out that this breaks "jsc" compilation for scripts with regular expressions in them. (Before, though, compilation worked but the scripts themselves did not -- we need some tests for this!)
Given a script "t.js" that has a constant regexp:
java -classpath rhino-all/build/libs/rhino-all-2.0.0-SNAPSHOT.jar org.mozilla.javascript.tools.jsc.Main -o out t.js
will throw IllegalStateException.
(Now, prior to this PR, the same compilation of a script would throw NullPointerException, but only at runtime, but we need to fix that too!)
Thanks!
There was a problem hiding this comment.
Apparently we don't get to this code any more in the PR, but this refers to the "_reInit" method which apparently was removed in this PR.
There was a problem hiding this comment.
I've removed that code.
| * regexp implementation reports anything wrong with an expression now, while there is still a | ||
| * script being compiled to report it against. If the implementation has no constant form, or | ||
| * there is no implementation at all, nothing is prepared and the literals are compiled when the | ||
| * class is defined by {@link #emitRegExpInit} instead. |
| if (constant != null) { | ||
| cfw.addLoadDynamicConstant(constant); | ||
| } else { | ||
| throw new IllegalStateException("null rgexp constant."); |
There was a problem hiding this comment.
typo in "rgexp", or at least according to common shortening of "regexp!"
| SYMBOL_REGULAR, | ||
| value.getName(), | ||
| CD_SYMBOL_KEY, | ||
| Integer.valueOf(System.identityHashCode(value))); |
There was a problem hiding this comment.
Is there a theoretical possibility of a collision on "identityHashCode"? I mean not back in the days of 32-bit systems, but I think yes. How do we fall back in that case?
There was a problem hiding this comment.
We'd only see a clash on heaps too large to use 32-bit references, which is okay up to a 32GB heap (because object alignment allows for compact references). We can theoretically get a clash in heaps above that, but for that to be a problem we'd need two symbols with the same name that are intended to be distinct to have an identity hash code collision.
If we're really worried then we could key a weak hash set of symbols, and if the symbol isn't in there then increment a counter and use that.
b8a891c to
cc3409a
Compare
|
I've expanded our class compiler test to cover regexes and template literals, and fixed up class compilation so these pass. |
|
Haven't forgotten about this! These break the "threejs" test case in the "rhino-benchmarks" repo:
Other than that I think this is really close... Thanks! |
|
It's not the only thing that breaks that benchmark. 😕 |
|
I found a couple of failures to deduplicate constants and failures to check for overflows in the constant pool maps when fixing things on the lexical scope branch. I think the flight planner benchmark has been mis-compiling its data file for a long time. Things broke on my lexical scoping branch because compilation changed just enough that the error was detected during compilation. The bug you found here is a short to int conversion issue when the constant index exceeds 0x7fff, fixing that shows we later cause the constant pool to become exhausted. Quite a few of the those benchmarks actually overflow, and if we are checking for this properly, fall back to the interpreter. We might want to think about a strategy for splitting out functions and stuff to subsidiary class files when things get too large. |
This branch has not been deployed
2 participants
When working on the lazy source provider API I wanted to be able to easily create a constant
SourceCodeProviderand have that de-duplicated by the class writer. The class file format allows for these constant dynamics in a similar manner to invoke dynamic bootstraps.This prototype is a little rough, Claude wrote some of it but got some of the symbol stuff very wrong so I mostly rewrote that and edited a bunch of the related tests.
SymbolKeywas chosen as a reasonable tractable example that we might conceivably want, butEagerSourceCodeProviderand other more complex constants would be my real uses for this.