Fix Java 15

[PimvanderLoos]

This PR fixes ProtocolLib's incompatibility with Java 15 on all versions of Minecraft after (and including) 1.13.0.

This incompatibility was caused by the fact that starting in Minecraft 1.13.0, the NetworkManager uses a lambda for Runnables/Callables that are used to send packets. Starting in Java 15, the classes generated by these lambdas are hidden.

Unlike for regular classes, you cannot use reflection to update final fields (regardless of Field#setAccessible(true)), as described in the JEP (see "Using a hidden class", point 3).

However, while changing final fields is not possible, reading them and accessing the class's constructor still works. Therefore, this PR introduces an ObjectReconstructor class that stores all the fields and the constructor of a class (at most once for a Callable and for a Runnable each). When a packet in a Callable/Runnable needs to be modified, this class is used to read the values of all the member variables, update the packet value, and construct a new Callable/Runnable instance. This new instance is then used for the remainder of the message handling process.

This PR fixes #978, fixes #1028.

[dmulloy2]

I like it! The ObjectReconstructor seems like a pretty elegant solution.

Have you tested with MC 1.8 and Java 8 to make sure it doesn't break backwards compatibility? Also it doesn't seem like there would be much of a performance or memory hit, but have you done any profiling?

[PimvanderLoos]

Thanks for your response!

This is what I've tested:

For every test, I used a simple test plugin that replaces a packet for falling sand with a packet for falling dirt (to ensure the packet-replacement code is fired). This allows me to visually see the result of changing the packet. Additionally, I used LibsDisguises as a more complete plugin.

Java 8 and 15.
Minecraft: 1.8.8/9, 1.12, 1.15, 1.16

On Minecraft 1.8.8/9 (server 1.8.8, client 1.8.9), this PR worked fine for both plugins using Java 8. However, I was unable to join the server when it ran Java 11+ (I did not test the versions in between). However, this was the case even without any plugins installed, so I guess the server just doesn't work on newer versions of Java.

On 1.12.2 and 1.16.4, both Java 8 and Java 15 worked without any issues.

I am not particularly experienced with profiling, but as far as I could see, the performance impact is negligible.

[PimvanderLoos]

While playing around with it a bit more, I just ran into an issue. I'm not entirely sure what it is yet, but please don't merge it yet. (wish I could mark it as a draft).

Update: The issue happens specifically on Tuinity. It happens because is that there is more than 1 type of Runnable being used (the second one being an io.netty.util.concurrent.AbstractEventExecutor$LazyRunnable).
When I first wrote this patch, I looked through nms to make sure that there was only 1 type of Runnable/Callable being used by this code. Because I couldn't find any other types being used, I used the static member fields instead of a map to minimize overhead.

What would be your preference? Add another field for the specific LazyRunnable (so we have 1 ObjectReconstructor for each of the following: LazyRunnable, Runnable, and Callable) or play it safe and use a map?

The code that caused the issue is Here + Here

[mikroskeem]

@PimvanderLoos I'm not really fond that you lifted my idea from the commit(s) I linked under #978. Please add appropriate credit.

[PimvanderLoos]

@mikroskeem I never actually saw your commit. I'd already written most of the changes, just forgot about it until I saw a message about a post in that thread. Most of the fun in this little project has been figuring out the root cause of the issue, after all ;)
The code itself is not particularly difficult, complicated, or groundbreaking, so it makes sense we went along somewhat similar paths.

I just checked if a PR/commit to master had been made to resolve the issue before I picked this back up.

[PimvanderLoos]

While going over @mikroskeem's commits, I saw that you use the old reflection method of updating values when the class isn't hidden. I thought this was a good idea, so I added such a system in this PR.
Also, Class#isHidden() will throw an exception on Java < 15, because that method was introduced in Java 15.

Also, I implemented the issue above regarding netty's LazyRunnables using hardcoded values for now (so that it at least works on all platforms that I'm aware of), but I'm still unsure if that's the best solution (hardcoding stuff never feels quite right).
I saw that @mikroskeem used a synchronized WeakHashMap. This has the advantage of removing all entries if/when a class is GC'ed, however, it being a Collections.SynchronizedMap, all actions on the map will be synchronized, so the throughput isn't great. Using a ConcurrentHashMap would resolve this issue as it allows concurrent reads, but it wouldn't automatically remove any GC'd keys (the classes).

My interpretation of the JEP is that these lambdas will not get GC'ed and simply get reused for as long as the NetworkManager class is loaded. They are defined with a STRONG relationship with the NetworkManager class. This would mean that using a ConcurrentHashMap does not have the risk of memory leak and give us better throughput than a synchronized WeakHashMap. What are your opinions on this matter?

[mikroskeem]

Also, Class#isHidden() will throw an exception on Java < 15, because that method was introduced in Java 15.

Indeed, I didn't bother making it compatible with Java <15 yet because I run Java 15 in production myself.

By the way, I would check if Class#isHidden() method is present, instead of parsing class version support property. Latter feels more fragile to me to be honest.

I'm still unsure if that's the best solution (hardcoding stuff never feels quite right)

I would indeed replace hardcoded fields with a map - you'll never know what changes are people making to their servers, sooner or later would someone open a PR to add a new case for a new class.

What are your opinions on this matter?

I haven't read JEP myself yet, so I wasn't sure when will lambdas get GC-ed. Hence I went with WeakHashMap & synchronizing on it. But if that interpretation is correct, then ConcurrentHashMap is suitable there.

[PimvanderLoos]

By the way, I would check if Class#isHidden() method is present, instead of parsing class version support property. Latter feels more fragile to me to be honest.

Why would it be more fragile? Surely all versions going forward will have support for hidden classes and the versions before didn't. That's why I ended up doing it this way, as it's shorter and cleaner than checking if the method exists.

I would indeed replace hardcoded fields with a map - you'll never know what changes are people making to their servers, sooner or later would someone open a PR to add a new case for a new class.

Yeah, I agree. I've switched to a ConcurrentHashMap now.

I haven't read JEP myself yet, so I wasn't sure when will lambdas get GC-ed. Hence I went with WeakHashMap & synchronizing on it. But if that interpretation is correct, then ConcurrentHashMap is suitable there.

The relevant part in the JEP is here:
In such cases, where the hidden class must be coterminous with a normal class, the STRONG option may be passed to Lookup::defineHiddenClass. This arranges for a hidden class to have the same strong relationship with its notional defining loader as a normal class has with its defining loader, which is to say, the hidden class will only be unloaded if its notional defining loader can be reclaimed.

And as you can see here, lambdas are indeed created using the STRONG option.

[mikroskeem]

Why would it be more fragile? Surely all versions going forward will have support for hidden classes and the versions before didn't. That's why I ended up doing it this way, as it's shorter and cleaner than checking if the method exists.

That works by assuming that the class where packet should be replaced is always a lambda. Some Spigot forks replace lambda with explicit inner class (or do other weird things), thus breaking the workaround right now. Calling Class#isHidden would solve that issue safely. But 🤷‍♂️

Previously I stated that simply checking the method presence is sufficient, but it's not.

As a side note, I should really keep myself away from issue trackers when tired.

[PimvanderLoos]

Well, as long as the number+type+order of member variables of a class match the number+type+order of arguments of the first constructor, the ObjectReconstructor is perfectly capable of reconstructing the object. This also includes explicit inner classes (as was the case in all versions before 1.13.0, which was the first version to use a lambda) so that's not a problem. Regarding some fork doing some weird stuff here, I'd argue that we'll cross that bridge when we get to it.

However, if the class isn't hidden, it would be preferable to use the simple update method instead of recreating the object. So how about something like this:

private static final Function<Class<?>, Boolean> IS_HIDDEN;
static {
    Function<Class<?>, Boolean> isHidden;
    try {
        isHidden = Class::isHidden;
    } catch (NoSuchMethodError e) {
        isHidden = (c) -> false;
    }
    IS_HIDDEN = isHidden;
}

...

if (original != changed)
    instance = (T) (IS_HIDDEN.apply(instance.getClass()) ?
            updatePacketMessageReconstruct(instance, changed, accessor) :
            updatePacketMessageSetReflection(instance, changed, accessor));

This way we ensure that only hidden classes are reconstructed and older versions of Java and any forks and versions of MC that use explicit inner classes use the simple update method. However, it means that ProtocolLib would have to be compiled on Java 15+, which might not be desirable and up to @dmulloy2 to decide.

Also, I ran a quick benchmark using JMH to see the actual differences between updating a value using reflection and reconstructing the object:

The benchmarkReconstruct benchmark used the ObjectReconstructor and the benchmarkSet benchmark used the DefaultFieldAccessor (both instantiated outside of the benchmark). Both updated a single value in a class with 5 variables.
Not included in these benchmarks is the overhead of using the ConcurrentHashMap (which only applies to the ObjectReconstructor). These are the results:

Benchmark                                                            Mode      Cnt       Score       Error   Units
BenchmarkRunner.benchmarkReconstruct                                thrpt      200       0.016 ±     0.001  ops/ns
BenchmarkRunner.benchmarkSet                                        thrpt      200       0.225 ±     0.001  ops/ns
BenchmarkRunner.benchmarkReconstruct                                 avgt      200      59.449 ±     0.193   ns/op
BenchmarkRunner.benchmarkSet                                         avgt      200       4.413 ±     0.022   ns/op
BenchmarkRunner.benchmarkReconstruct                               sample  5845053      84.671 ±     0.851   ns/op
BenchmarkRunner.benchmarkReconstruct:benchmarkReconstruct·p0.00    sample               70.000               ns/op
BenchmarkRunner.benchmarkReconstruct:benchmarkReconstruct·p0.50    sample               80.000               ns/op
BenchmarkRunner.benchmarkReconstruct:benchmarkReconstruct·p0.90    sample               90.000               ns/op
BenchmarkRunner.benchmarkReconstruct:benchmarkReconstruct·p0.95    sample               90.000               ns/op
BenchmarkRunner.benchmarkReconstruct:benchmarkReconstruct·p0.99    sample              130.000               ns/op
BenchmarkRunner.benchmarkReconstruct:benchmarkReconstruct·p0.999   sample              722.000               ns/op
BenchmarkRunner.benchmarkReconstruct:benchmarkReconstruct·p0.9999  sample             4328.000               ns/op
BenchmarkRunner.benchmarkReconstruct:benchmarkReconstruct·p1.00    sample           665600.000               ns/op
BenchmarkRunner.benchmarkSet                                       sample  6344783      27.254 ±     0.048   ns/op
BenchmarkRunner.benchmarkSet:benchmarkSet·p0.00                    sample               20.000               ns/op
BenchmarkRunner.benchmarkSet:benchmarkSet·p0.50                    sample               30.000               ns/op
BenchmarkRunner.benchmarkSet:benchmarkSet·p0.90                    sample               30.000               ns/op
BenchmarkRunner.benchmarkSet:benchmarkSet·p0.95                    sample               31.000               ns/op
BenchmarkRunner.benchmarkSet:benchmarkSet·p0.99                    sample               40.000               ns/op
BenchmarkRunner.benchmarkSet:benchmarkSet·p0.999                   sample               80.000               ns/op
BenchmarkRunner.benchmarkSet:benchmarkSet·p0.9999                  sample             1853.043               ns/op
BenchmarkRunner.benchmarkSet:benchmarkSet·p1.00                    sample            19264.000               ns/op
BenchmarkRunner.benchmarkReconstruct                                   ss       10  213423.000 ± 19783.442   ns/op
BenchmarkRunner.benchmarkSet                                           ss       10   87309.500 ± 17424.185   ns/op 

As you can see, using the DefaultFieldAccessor is quite a bit faster than the ObjectReconstructor, though on average still on the nanosecond scale, so probably not noticeably so. Nonetheless, I think this is the best solution if requiring Java 15+ for compilation is acceptable.

[mikroskeem]

However, it means that ProtocolLib would have to be compiled on Java 15+, which might not be desirable

It's as easy as compiling ProtocolLib with newer JDK, so I don't see a problem in that. If that's not viable for dmulloy2, then a different solution should be found.

Alternative to compiling with Java 15 is unsurprisingly using reflection. I threw quick solution together yesterday to fix the object construction issue I ran into yesterday. @PimvanderLoos since you already have working JMH benchmark, then could you throw my changes in and see how it performs?

As a side note, I would use Predicate instead of Function, as that'll avoid using Boolean object.

[PimvanderLoos]

Yeah, I wanted to avoid using reflection, but it's only fair if I benchmark that as well.
I only benchmarked the average time, because these benchmarks take quite a while to run, but if you want more info, I can run them again with more modes enabled (but I won't be able to post the results until tonight/tomorrow).

What I did for the benchmarks:

1. Use this (your) code (i.e. access Class#isHidden() with reflection) using a Method and Class object defined outside of the benchmark.
2. Use a Predicate for Class::isHidden to determine its status (again both the Predicate and the Class objects defined outside of the benchmark).
3. The same as 2, but using a Function<Class<?>, Boolean> instead of a predicate, as I was curious about the difference.
4. Reconstruct the object using the ObjectReconstructor as normal, but now including the map lookup (computeIfAbsent) as I was curious to see the impact of using a map (again, everything defined outside of the benchmark).
5. / 6. I noticed that returning something from a function takes longer than not returning something (seems obvious in hindsight). The difference isn't that big (certainly doesn't change the outcome), but I thought it'd be better to rerun the Reconstruct and Set benchmarks but now let them return the appropriate value (all benchmarks listed below do so).

Results:

Benchmark                                       Mode  Cnt   Score   Error  Units
1) BenchmarkRunner.benchmarkIsHiddenReflection  avgt  200   5.357 ± 0.024  ns/op
3) BenchmarkRunner.benchmarkIsHiddenPredicate   avgt  200   3.830 ± 0.008  ns/op
2) BenchmarkRunner.benchmarkIsHiddenFunction    avgt  200   3.825 ± 0.010  ns/op
4) BenchmarkRunner.benchmarkReconstructFromMap  avgt  200  74.221 ± 0.409  ns/op
5) BenchmarkRunner.benchmarkReconstruct         avgt  200  60.723 ± 0.224  ns/op
6) BenchmarkRunner.benchmarkSet                 avgt  200   7.477 ± 0.026  ns/op

As you can see, accessing the isHidden method using reflection is somewhat slower than using a simple predicate/function. However, the difference is about 1.5ns on average, so it's definitely worth it if it can be used to avoid the much, much slower ReconstructFromMap operation (roughly: isHidden + Set = 12.834 ns vs 74.221 ns).
With such a minor difference, I'd argue it's also worth it to use this method in favor of the predicate for the convenience of being able to compile ProtocolLib with Java 8+ instead of only with Java 15+.

For completeness' sake: I'm using JMH's default values with the exception of the OutputTimeUnit, which was set to nanoseconds. All benchmarks were run on a 3900x.

[PimvanderLoos]

I applied @mikroskeem's commit that switches to using reflection to check if a class is hidden or not, as based on the benchmarks, this seems like the way to go.

Just to make sure everything still works as expected, I ran several manual tests.
I tested on the following MC versions: 1.8.8/9 (oldest supported platform), 1.12.2 (last version using explicit Runnables that also supports J15), and 1.16.4.
I tested using the following Java versions: J8, J11, and J15.

For 1.8, I only ran the test on J8, as J11+ doesn't work (I think J9+ doesn't work either, but I don't have either J9 or J10).

For every test, I used a simple test plugin that replaces packets of falling sand with packets for falling dirt. On MC 1.12+, I also used LibsDisguises.

For MC 1.16, I used both Spigot and Tuinity (and for Tuinity, only J11+, as it doesn't run lower versions).

In total, I tested this PR on 9 different server/Java combinations and found no issues.
Additionally, I ran the same test on MC 1.16.4 with J11 using the current master branch to use as a baseline.

In conclusion, I think this PR is now ready.

[fabianmakila]

Been running this on our 1.16.4 paper servers running OpenJDK 15.0.1. Seems to work great, no problems so far.

[RavenskyPaul]

Been running this on our 1.16.4 paper servers running OpenJDK 15.0.1. Seems to work great, no problems so far.

Is it possible for you to insert a link to download your ProtocolLib.jar here?

[dmulloy2]

Perfect, thank you for being so thorough!

[dmulloy2]

Available in the latest dev build: https://ci.dmulloy2.net/job/ProtocolLib/lastSuccessfulBuild/

[PimvanderLoos]

Thanks for merging it! And @mikroskeem, it has been a pleasure working with you on this. :)