-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Remove boxing when loading and storing values in "def" fields/arrays, remove boxing onsimple method calls of "def" methods #18359
Remove boxing when loading and storing values in "def" fields/arrays, remove boxing onsimple method calls of "def" methods #18359
Conversation
I dumped the bytecode of several array tests (field stores are hard at the moment; they are also completely untested, as far as I know): First example: This creates the following invokedynamic:
As you see there is no boxing involved anymore, the value is pushed unmodified to stack. It also works if the value to Store is originally a String, although it does not really involves boxing, but you see that type passed to invokedyanmic is preserved:
|
Thanks for killing more boxing! I will look when i get home. |
Thanks @rmuir! |
…ic for consistency with other method calls
ec712d4
to
604bcd9
Compare
New version, I nuked the old one. This is now clean. The new superclass of |
I also was able to remove boxing from loads and method calls - as long as they are simple expressions. Still not solved are compound statements (increments,...). I have to first understand The new code works similar to before: It checks if the link node used for read is a |
Example: Bytecode for the part
|
…s also a bug with the size of LDefArray: fixed
I also fixed a bug with compound statements and made them work with For compound statements, we can unfortunately make no type propagation, because we load from DEF and store from DEF, so there is no more type information. E.g., if you increment a DEF, it will always box. But that has nothing to do with this issue (invokedynamic). The other thing not yet solved is missing type information for statements like This PR now solves all the "simple" cases and also fixes a bug with def arrays and stack size. |
Thank you, I think i hit this one when experimenting the other day! |
OK, this would be great to look into. Already this patch helps, for example our script in the nightly benchmark:
We see removal of boxing for the log(abs(population)), instead of:
we now see:
Unfortunately the optimization ends there for now (but this is progress!). I think part of the problem is that for the larger expression, painless has no idea that it needs to return a double in this case, so at the end of the day always returns I will look into trying to clean this up, i hate that we don't know our return type when being compiled, i think its grossly unfair of the scripting api to do this. If we can fix it, then I think more type information would bubble up the tree and we'd kill more of the overhead. |
There is some parts missing in painless:
One idea to fix this would be a two-phase analyze: In the first step of analyze the code is checked for inconsistenceies and every node would assign the "I'd be happy to get that input and output types". In a second pass of optimize the information collected in first phase would be used to adopt types also the other way round: if a sub node returns Def only after first phase, but the acceptor reported that it may accept double or float, make the inner node adopt its return type. The last step would then be writing of byte code. |
Thats a separate issue. I would make 2 abstract Executables: One that returns Object and one that returns double. Painless checks while compiling which one to implement and returns the double one if possible. I think this should be possible to implement with some logic in the Writer if the "actual" type of the root node is primitive. |
Also a bit unrelated to this issue, but we may investigate it in a separate issue: If boxing is needed (we cannot avoid it everywhere, like We already have a This is not a cache - it is static, it just saves instances of all boxed constants found in code. |
|
||
AExpression index; | ||
|
||
LDefArray(final int line, final String location, final AExpression index) { | ||
super(line, location, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
At Uwe, thanks for tackling this! I left a couple of comments. |
@uschindler I really like your idea of the two-passes to collect all known type information! |
Yes, I think this should be a separate issue. This issue is just to improve the situation, but a 2 pass analyze might be the best option, although this won't help for all cases. Sometimes it is impossible to catch all. |
Oh yeah, definitely a separate issue. :) |
Hi Jack, I fixed the issue with the "this.actual" value so chained assignment works correct. So I need to change this actual output to the (not) casted one. The next part in chained expression is then casting the duped value afterwards. Now it passes. |
@uschindler This looks great, if you're happy with it, I'm happy with it. +1 I'm happy to commit as-is unless there was anything else you wanted to add or change. |
expression = expression.cast(settings, definition, variables); | ||
|
||
statement = true; | ||
actual = read ? last.after : definition.voidType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the regular case it should be the same, but moved into the else block
Hi Jack, you see this was moved up to else block!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Thank you.
Sorry for the confusion on this.actual versus expression.actual. Took a bit for the caffeine to kick in! |
@uschindler Are you ready for me to commit? |
No problem :) Maybe we should change the assignments tos use this as prefix also in source code. As EChain is also an expression, this makes clear if the inner expression or this is meant by the assignments. I can fix this, OK? |
@uschindler Yeah, please do. Just let me know whenever you're ready. |
…nner "expression" is affected
OK, I am done. Thanks for help and finding the remaining issue with the cast. We should really work on more tests for more corner cases. In addition, I was talking with @rmuir about some way to check that "optimizations" in bytecode survive refactorings. One idea is to dump the bytecode to a String in tests using debugger class and look for patterns in it (e.g. for this issue make sure that it does not box by looking for "java/lang/Integer.valueOf" and fail test if it occurs in bytecode). Alternatively use forbiddenapis' API (as test dependency) and invoke |
@uschindler Thanks again for working on this issue! I really like the idea of checking for the optimizations using the patterns from the bytecode. Definitely need to ensure these changes stay in the future. |
I finally managed to get the field and array stores to
def
fields or arrays no longer box. The problem to solve was to hack intoEChain.analyzeWrite
and promote the type the other way round:def
type, the code changes the return type of the expression (of which result should be stored) and disables casting the usual way. After that the return type of the expression gets promoted to directly to the store node.write()
was adapted to use the promoted type in the descriptor (this change was missing in my last PR already).In addition in this PR I made all invokedynamic calls use the
GeneratorAdapter
method name instead on the underlying visitor's method name. This makes all invokes named the same throughout codebase.Another change is removal of a useless List -> array clone.