Remove the lamba factory method. Use the static field or constructor directly. #83
Conversation
|
I suspect this one might have more feedback as it's much more involved. Let me know what you think! |
|
Oh, and this saves 860 methods references from our app which is ~1.4%! |
|
I found a bug with this pull request. I added a test for it to master (commit 365b819). |
| // 5: invokevirtual #3 // Method java/lang/Object.getClass:()Ljava/lang/Class; | ||
| // 8: pop | ||
| // 9: invokedynamic #4, 0 // InvokeDynamic #0:call:(Ljava/lang/String;)Ljava/util/concurrent/Callable; | ||
| // Attempt to match whether these (4, 5, and 8) are the three preceding bytecodes and remove them if so. |
There was a problem hiding this comment.
Those three instructions should not do any harm, so why are they being removed?
There was a problem hiding this comment.
They screw up the calculation of how far "up" to place the new and dup instructions because they are between the argument loads and invokedynamic call. Instead of removing I can just walk backwards through the instructions until N loads are seen (where N is the number of arguments required).
There was a problem hiding this comment.
I have updated the PR with different logic here which removes the special casing of this bytecode sequence.
|
Thanks for the test. I rebased the branch locally on your failing test to take a look. I'm about to hop on planes for 8 hours so hopefully will have something by tomorrow. |
JakeWharton
force-pushed
the
jw/remove-factory-method
branch
from
30ed485
to
569b190
Compare
Updated and fixed! |
JakeWharton
force-pushed
the
jw/remove-factory-method
branch
2 times, most recently
from
50db7ed
to
83f2c4e
Compare
JakeWharton
force-pushed
the
jw/remove-factory-method
branch
from
83f2c4e
to
93beb97
Compare
|
This change is still fraught with problems and may not be viable to complete. Going to close while I work on it more. |
Refs #81 item 3.