GEODE-866: Converting calls of vm.invoke to lambdas #98
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This makes writing tests easier, it will get converted to an unchecked exception. Adding throws Exception to some tests that invoked run directory on SerializableRunnable.
These methods should no longer be used. Unfortunately, there are some calls that could not be automatically converted, so they cannot be completely removed.
This is an automatic conversion of many of the calls to vm.invoke that
take a string to instead use a lambda.
This was done using the following perl search/replace expressions on all
dunit tests.
perl -i -0pe 's/.\s*invoke(?:Boolean|Int|Long|)(Async|)\(\s*(\w+)(?:\.class|\.getClass\(\))\s*,\s*"([^"]+)",\s*new\s+Object\[\]\s*{([^}]*)}\s*\)/.invoke$1(() -> $2.$3($4))/mg' $1
perl -i -0pe 's/.\s*invoke(?:Boolean|Int|Long|)(Async|)\(\s*(\w+)(?:\.class|\.getClass\(\))\s*,\s*"([^"]+)"\s*\)/.invoke$1(() -> $2.$3())/mg' $1
perl -i -0pe 's/.\s*invoke(?:Boolean|Int|Long|)(Async|)\(\s*getClass\(\)\s*,\s*"([^"]+)",\s*new\s+Object\[\]\s*{([^}]*)}\s*\)/.invoke$1(() -> $2($3))/mg' $1
perl -i -0pe 's/.\s*invoke(?:Boolean|Int|Long|)(Async|)\(\s*getClass\(\)\s*,\s*"([^"]+)"\s*\)/.invoke$1(() -> $2())/mg' $1
Now that we are using lambdas, variables passed to the lambda should be effectively final.
These are places where the code invoke was calling a method that doesn't exist. These tests were either not running, or in some cases never checking the return value of an invokeAsync call.
These couldn't be automatically converted. I've changed them to lambdas.
Fixing various issues being checked by the compiler: - Changing generics to match callers - Calling invoke on the correct class - Fixing some ambiguous casts. - Fixing some method visibility issues. - Removing extra comma after parameters
After converting these tests to use lambdas, it appears that the large number of lambdas are overflowing some limit of the compiler. By extracting methods for common lambdas, the overall number of lambda classes required is reduced for these tests.
A bunch of tests were failing because the location in which static fields are evaluated has changed. Before, a static in the parameter list was evaluated on the controller. Now it is evaluated on the remote VM. Converting the statics to fields or local variables so they are captured and serialized.
These are a few places I missed in my last checkin.
|
BTW - I can push this change myself, I just thought it would be easier to review as a PR than a reviewboard request. If I get a couple of +1s and no -1s in the next day or so I'll push it. |
|
+1 Awesome! |
|
+1 |
|
+1 changes look great |
|
+1 very nice |
|
+1 great |
bschuchardt
pushed a commit
to bschuchardt/geode
that referenced
this pull request
Jul 10, 2020
By reducing thread count to 10x CPUs we found that on the same instances we only lose about 4% of throughput but seem more than 40% improvement in latency. More importantly the variation between runs drops significantly. This variation was greater than 10% on Java 11 and 3% on Java 8. When lowered to 10x threads we see closer to 1% variation for both Java 11 and 8.
robbadler
pushed a commit
to smgoller/geode
that referenced
this pull request
Jul 12, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I automatically converted many of the calls of vm.invoke(Class, String) to use a lambda instead, as discussed. Please review this PR - I think it will be easier to review the individual commits than a review board post.
There were a few roadblocks along the way:
Not all vm.invoke calls are gone, unfortunately. There are a number of places that could not be automatically converted - mostly where the code built up an object array in a weird way and then passed it to invoke. We'll have to manually fix these.