Skip to content

Explicitly construct array for dispatching events#405

Closed
xxDark wants to merge 1 commit into
PaperMC:masterfrom
xxDark:event
Closed

Explicitly construct array for dispatching events#405
xxDark wants to merge 1 commit into
PaperMC:masterfrom
xxDark:event

Conversation

@xxDark
Copy link
Copy Markdown
Contributor

@xxDark xxDark commented Jul 19, 2019

No description provided.

@Black-Hole
Copy link
Copy Markdown
Contributor

Black-Hole commented Jul 19, 2019

What's the purpose of this commit? Does it lower cpu or memory consumption? If so, do you have some some tests that shows this?

@xxDark
Copy link
Copy Markdown
Contributor Author

xxDark commented Jul 19, 2019

What's the purpose of this commit? Does it lower cpu or memory consumption? If so, do you have some some tests that shows this?

Reduces array creation caused by invoking events via reflection, since java.lang.Method.invoke(...) creates new array on each invocation, this commit replaces it with direct MethodAccessor's, and array is created only once.

Will provide tests maybe tomorrow, cuz I'm a bit tired rn

@MrIvanPlays
Copy link
Copy Markdown

You are missing a bunch of // Waterfalls, also why you exclude classes from compiling? One more thing: you are making new classes which are specific for waterfall under the net.md_5 package, while you should make it under io.github.waterfallmc.waterfall

@Janmm14
Copy link
Copy Markdown
Contributor

Janmm14 commented Jul 20, 2019

The purpose of those classes excluded from jar packing is that he apparanrly didnt find a library for these jre-internal classes.

@xxDark
Copy link
Copy Markdown
Contributor Author

xxDark commented Jul 20, 2019

The purpose of those classes excluded from jar packing is that he apparanrly didnt find a library for these jre-internal classes.

Nope, sun.reflect is jdk.internal.reflect since jdk 9, that's why dummy classes was added.

@electronicboy
Copy link
Copy Markdown
Member

java.lang.IllegalAccessError: class net.md_5.bungee.event.InternalReflectionFactory$1 (in unnamed module @0x32a068d1) cannot access class jdk.internal.reflect.ReflectionFactory (in module java.base) because module java.base does not export jdk.internal.reflect to unnamed module @0x32a068d1

@xxDark
Copy link
Copy Markdown
Contributor Author

xxDark commented Aug 21, 2019

java.lang.IllegalAccessError: class net.md_5.bungee.event.InternalReflectionFactory$1 (in unnamed module @0x32a068d1) cannot access class jdk.internal.reflect.ReflectionFactory (in module java.base) because module java.base does not export jdk.internal.reflect to unnamed module @0x32a068d1

You closed PR, and did not allow to fix this?
https://www.youtube.com/watch?v=h3uBr0CCm58

@electronicboy
Copy link
Copy Markdown
Member

If you have a fix, you're more than welcome to open a new PR, we cannot use those internal classes however.

@Janmm14
Copy link
Copy Markdown
Contributor

Janmm14 commented Aug 22, 2019

Is 99.99% also not Java 8 compatible
Java 9+ compatibility for this would require a startup flag to import internal jdk modules

@MarkTwoFive
Copy link
Copy Markdown

You could try putting everything into the classpath instead of the module path. Reflection should work as expected then. At least thats what we see in Java 11.

@electronicboy
Copy link
Copy Markdown
Member

This PR breaks in several ways in java 9+, breaking compilation is a major one, and indicates that this is not compatible with 9+ as a whole; I'm not really interested in trying to hack around this, or require that users potentially have to start the jar differently

@MarkTwoFive
Copy link
Copy Markdown

Yes, I completely agree with that. I just wanted to say that this option is available for use when automated modules do not allow reflective access for example for older plugins that do not have a module information file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants