[Util/Method] Spawn ItemFrames (NMS/Reflection)

Discussion in 'Resources' started by Guichaguri, Jul 26, 2014.

Thread Status:
Not open for further replies.
  1. Offline

    Guichaguri

    I was having many problems spawning and positioning an ItemFrame with the Bukkit API, so I created a method that let me spawn ItemFrames easily.

    The method with reflection(multiversion): (Pastebin)
    Code:java
    1. private static String mcPackage = null;
    2. private static Class classItemFrame = null;
    3. private static Constructor constructor = null;
    4. private static Method worldGetHandle = null;
    5. private static Method worldAddEntity = null;
    6. private static Method itemFrameGetBukkitEntity = null;
    7. public static ItemFrame spawnItemFrame(Location loc, BlockFace bf) throws Exception {
    8. int side = 0;
    9. if(bf == BlockFace.SOUTH) {
    10. side = 0;
    11. } else if(bf == BlockFace.WEST) {
    12. side = 1;
    13. } else if(bf == BlockFace.NORTH) {
    14. side = 2;
    15. } else if(bf == BlockFace.EAST) {
    16. side = 3;
    17. }
    18. World w = loc.getWorld();
    19.  
    20. // CACHE CRAFTWORLD METHOD
    21. if(worldGetHandle == null) worldGetHandle = w.getClass().getMethod("getHandle");
    22.  
    23. // GET NMS WORLD
    24. Object mcWorld = worldGetHandle.invoke(w);
    25.  
    26. // CACHE NMS ELEMENTS
    27. if(mcPackage == null) mcPackage = mcWorld.getClass().getPackage().getName();
    28. if(classItemFrame == null) classItemFrame = Class.forName(mcPackage + ".EntityItemFrame");
    29. if(worldAddEntity == null) worldAddEntity = mcWorld.getClass().getMethod("addEntity", classItemFrame.getSuperclass().getSuperclass());
    30. if(itemFrameGetBukkitEntity == null) itemFrameGetBukkitEntity = classItemFrame.getMethod("getBukkitEntity");
    31. if(constructor == null) constructor = classItemFrame.getConstructor(mcWorld.getClass().getSuperclass(), int.class, int.class, int.class, int.class);
    32.  
    33. // CREATE A NEW ITEMFRAME
    34. Object mcItemFrame = constructor.newInstance(mcWorld, loc.getBlockX(), loc.getBlockY(), loc.getBlockZ(), side);
    35. // ADDS THE ITEMFRAME TO THE WORLD
    36. worldAddEntity.invoke(mcWorld, mcItemFrame);
    37. // FINALLY, RETURNS THE BUKKIT ITEMFRAME
    38. return (ItemFrame)classItemFrame.getMethod("getBukkitEntity").invoke(mcItemFrame);
    39. }


    The method without reflection: (Pastebin)
    Code:java
    1. public static ItemFrame spawnItemFrame(Location loc, BlockFace bf) {
    2. int side = 0;
    3. if(bf == BlockFace.SOUTH) {
    4. side = 0;
    5. } else if(bf == BlockFace.WEST) {
    6. side = 1;
    7. } else if(bf == BlockFace.NORTH) {
    8. side = 2;
    9. } else if(bf == BlockFace.EAST) {
    10. side = 3;
    11. }
    12. // GET NMS WORLD
    13. WorldServer w = ((CraftWorld)loc.getWorld()).getHandle();
    14. // CREATE A NEW ITEMFRAME
    15. EntityItemFrame entity = new EntityItemFrame(((CraftWorld)loc.getWorld()).getHandle(), loc.getBlockX(), loc.getBlockY(), loc.getBlockZ(), side);
    16. // ADDS THE ITEMFRAME TO THE WORLD
    17. w.addEntity(entity);
    18. // FINALLY, RETURNS THE BUKKIT ITEMFRAME
    19. return (ItemFrame)entity.getBukkitEntity();
    20. }


    Example:
    [​IMG]
    Code:
    Location cobblestoneBlock = ...
     
    ItemFrame itemframe = spawnItemFrame(cobblestoneBlock, BlockFace.WEST);
    I hope this helped you.
     
  2. Offline

    xTrollxDudex

    Guichaguri
    If it's such a great "ease" to use NMS and reflection without thinking about it, think again. Examine the cb source yourself and think again. Never use NMS for what the Bukkit API provides, not only is it safe, it is also convenient and fast, unlike 3x slow reflection... After looking at the drawbacks, think again.
     
    Mr360zack likes this.
  3. Offline

    Guichaguri


    I'll give you some examples.
    My code:
    Code:java
    1. ItemFrame itemframe = (ItemFrame)cobblestoneBlock.getWorld().spawn(cobblestoneBlock,ItemFrame.class);
    2. itemframe.setFacingDirection(BlockFace.WEST, true);

    Result (trying to spawn an itemframe in a floating cobblestone block):
    Code:
    IllegalArgumentException: Cannot spawn hanging entity for org.bukkit.entity.ItemFrame at Location{world=CraftWorld{name=world},x=-742.5265427469436,y=5.0,z=820.6231225572004,pitch=87.0,yaw=-202.34598}
    Another test (trying to spawn the ItemFrame facing west on the cobblestone block, it not crashed this time because of the block behind the cobblestone):
    [​IMG]

    THATS why.

    Also, I left a non-reflection version.
     
  4. Offline

    IDragonfire

    What happen if you spawn a block behind, create the frame over bukkit and remove the block?
     
  5. Offline

    PandazNWafflez

    xTrollxDudex If you think reflection is 3x slower than a method call you need to seriously reconsider your understanding of Java and the JVM. While reflection is normally about 1.5x speed on modern JVMs, it is actually possible for reflection to be FASTER than normal code. Don't believe me? I just benchmarked it, and I'll even leave the code here if you want to try it yourself.
    Code:java
    1. public class Main {
    2. public static void main(String[] args) throws Exception {
    3. reg();
    4. ref();
    5. }
    6.  
    7. public static void reg() throws Exception {
    8. long start = System.nanoTime();
    9. A a = new A();
    10. for (int i = 0; i < 1000000; i++) {
    11. a.lol();
    12. }
    13. System.out.println((System.nanoTime() - start) / 1000000);
    14. }
    15.  
    16. public static void ref() throws Exception {
    17. long start = System.nanoTime();
    18. Class<A> clazz = (Class<A>) Class.forName("test.A");
    19. Method m = clazz.getDeclaredMethod("lol");
    20. A a = clazz.newInstance();
    21. for (int i = 0; i < 1000000; i++) {
    22. m.invoke(a);
    23. }
    24. System.out.println((System.nanoTime() - start) / 1000000);
    25. }
    26. }
    27.  
    28. public class A {
    29. public A() {
    30. }
    31.  
    32. public void lol() {
    33. new ArrayList<A>().add(this);
    34. new LinkedList<A>().add(this);
    35. }
    36. }

    My average results were 29ms for regular, 23ms for reflection. This is because actually invoking methods, getting and setting fields, etc with reflection is very fast. Notice that instantiation and lookup was done outside of the loop, and done once for each time. Adding this into the loop causes a significant performance drop in both, but in reflection more so than in regular code.

    While it is likely that the code in the OP takes about twice as long as without reflection, if he were to cache the values for Class.forName, getConstructor and getMethod and not keep doing it, it would be significantly faster than it currently is and wouldn't be a big downgrade on normal code.

    Of course, all debate about the speed of reflection is rather pointless - the reflection is there for people who don't want to build on CraftBukkit, obviously - as you can see you don't need any NMS / OBC imports should you use the reflection version.

    Note to OP: Cache the values for Class.forName, getConstructor and getMethod to make the reflection way faster, if you even care :p
     
    Guichaguri likes this.
  6. Offline

    Guichaguri

    PandazNWafflez
    I cached, thanks for the suggestion. :)

    Also, maybe I'll create a PR (Pull Request) solving this bug.

    IDragonfire
    Removing the block behind will cause the ItemFrame to update and breaking and this is a ugly hack.
     
  7. Offline

    xTrollxDudex

    False. I don't believe you. I have done a much more extensive test discussed with 5 other developers, confirmed and re-tested on 2 different machines, a 3rd confirmation machine using 4 different testing methods (2 or which were unfortunately corrupt), and the results are conclusive. Yes, reflection IS 3x slower than a normal nonreflective call.

    It is not possible even under a severely flawed test for reflection to be faster than normal invocation, without using core API under sun packages.

    I consider my Java and understanding to be superior to yours, thank you very much.

    I have my results under a blog post I made here:
    http://agenttroll.github.io/blog/blog/2014/07/20/fast-reflection/

    By the way, do you have a really slow machine, or really inaccurate tests? Even on my machine, the times were much faster.

    By the way, benchmarking is not conducted in a single simple test. This was developed and tested over a week long. Your test is also flawed in several ways.

    Not only that, but you completely disregarded the JIT compiler, (I had 3 methods of preventing the JIT from affecting my tests) therefore, your understanding of the JVM is incomplete and inaccurate. Also don't believe your benchmarks, they were conducted in a single session, not good.
     
  8. Offline

    PandazNWafflez

    xTrollxDudex The fact you even wrote any of that is ridiculous, really. You have done nothing but show a bad understanding of the JVM. Let me explain it in simple terms. The JVM performs optimizations (yes it's a long word, get used to it). On modern JVMs in particular, these optimizations are very effective, and in some cases can increase speed by up to 35x in reflection going off of multiple iterations. Maybe you have a different JVM to other people (I have Java 8), so your optimizations aren't as efficient.

    I can guarantee right now that either you didn't perform an 'extensive test discussed with 5 other developers, confirmed and re-tested on 2 different machines, a 3rd confirmation machine, and the results are all conclusive', or your test was awful. The simple fact that you wrote '2 different machines, a 3rd confirmation machine' instead of '3 different machines' indicates to me that you are clearly either trolling (ironic eh) or trying to make it seem like your tests are 's00per l337' because you can use the word 'confirmation' in a sentence - despite the fact your sentence's grammar was utterly appalling.

    Anyway, here is something to improve your clearly limited understanding of the performance of reflection. Lookups (Class.forName("x");) and instantiation (clazz.newInstance(); / clazz.getConstructor(...).newInstance();) tend to be slow. This is true. However, invocations and gets / sets aren't slow at all. Take the following different comparisons between reflection (java.lang) and normal code:
    Code:java
    1. REF1: for (int i = 0; i < 100000; i++) {
    2. Class clazz = Class.forName("lol");
    3. lol l = clazz.newInstance();
    4. l.getDeclaredMethod("lol").invoke(l);
    5. }
    6.  
    7. REG1: for (int i = 0; i < 100000; i++) {
    8. lol l = new lol();
    9. l.lol();
    10. }
    11.  
    12. Class clazz = Class.forName("lol");
    13. REF2: for (int i = 0; i < 100000; i++) {
    14. lol l = clazz.newInstance();
    15. l.getDeclaredMethod("lol").invoke(l);
    16. }
    17.  
    18. REG2: for (int i = 0; i < 100000; i++) {
    19. lol l = new lol();
    20. l.lol();
    21. }
    22.  
    23. Clazz clazz = Class.forName("lol");
    24. lol l = clazz.newInstance();
    25. Method m = l.getDeclaredMethod("lol");
    26. REF3: for (int i = 0; i < 100000; i++) {
    27. m.invoke(l);
    28. }
    29.  
    30. lol l = new lol();
    31. REG3: for (int i = 0; i < 100000; i++) {
    32. l.lol();
    33. }

    As you go down, the difference between the two gets smaller. In the first example (REF1 and REG1), the reflection was roughly 2x as slow. In the second (REF2 and REG2) the reflection was roughly 1.5x as slow. In the final comparison, the difference was negligible and sometimes reflection was faster. This is because when something is called loads, the JVM optimizes in order to increase performance.

    If you were to check the OP's code vs the Craftbukkit code, the different would definitely not be 3x, as you earlier said.

    BTW Guichaguri, you didn't cache 'classItemFrame.getMethod("getBukkitEntity");' ;) :p

    xTrollxDudex Oh wow, your edit... JVM optimizes by not calling methods which don't do anything, and it can even remove for loops which do nothing. Your empty dummy.doWork() method in your first tests? Probably not called most of the time.

    You also mention that my test was bad - essentially, you are saying, because it's one method. This has negligible impact. In fact, all your classes, variables and methods on your blog post don't improve the benchmark quality at all. You're also talking about one nanosecond differences as if they are actually a difference - no code executes in the same amount of nanoseconds every time.

    Sure, in your final tests you managed to increment an integer in your doWork() method, but considering you ran just 50 iterations your results are fairly meaningless, considering the JVM didn't have time to optimize.

    Finally, you say you had methods of stopping JIT from affecting your tests? You know, that's not a good thing? Why on earth would you want to benchmark something and force the results to be LESS accurate than they would normally be? That's just insanity, and proves that your results are wrong.

    You should also note than using any sun packages is bad bad news, especially for servers, like Bukkit ones, because you can't guarantee it will be there in every JVM, and there is a chance things can be removed between Java updates.

    From the Oracle website (they own Java, in case you didn't know):
    Don't Use Sun (open)
    If a Java program directly calls only API in these packages, it will operate on all Java-compatible platforms, regardless of the underlying OS platform.

    A Java program that directly calls into sun.* packages is not guaranteed to work on all Java-compatible platforms. In fact, such a program is not guaranteed to work even in future versions on the same platform.

    Each company that implements the Java platform will do so in their own private way. The classes in sun.* are present in the JDK to support Oracle's implementation of the Java platform: the sun.* classes are what make the Java platform classes work "under the covers" for Oracle's JDK. These classes will not in general be present on another vendor's Java platform. If your Java program asks for a class "sun.package.Foo" by name, it may fail with ClassNotFoundError, and you will have lost a major advantage of developing in Java.

    Technically, nothing prevents your program from calling into sun.* by name. From one release to another, these classes may be removed, or they may be moved from one package to another, and it's fairly likely that their interface (method names and signatures) will change. (From Oracle's point of view, since we are committed to maintaining the Java platform, we need to be able to change sun.* to refine and enhance the platform.) In this case, even if you are willing to run only on Oracle's implementation, you run the risk of a new version of the implementation breaking your program.

    In general, writing java programs that rely on sun.* is risky: those classes are not portable, and are not supported.


    EDIT by Moderator: merged posts, please use the edit button instead of double posting.
     
    Last edited by a moderator: Jun 9, 2016
  9. Offline

    raGan.

    xTrollxDudex
    I hate to jump into your argument, but it seems like ending it before it really starts would be to most sensible thing to do.
    PandazNWafflez
    There are countles articles about benchmarking. Those very articles suggest that yours is flawed. Your unsupported claims about JVM optimizations (not that it doesn't happen, you are just very vague) most certainly do not improve credibility of your tests. You apparently haven't read the troll's blog post so you can't be sure how extensive his testing actually was. (might no longer be the case) I am not trying to take his side and claim that reflection is at least 3 times slower, but I also haven't seen a proper proof of reflection being faster than regular calls.

    Wherever the true might be, let's stay on topic here. OP just provided a little utility to help with a flaw in bukkit api. Let's talk about that instead.

    Edit:
    "Finally, you say you had methods of stopping JIT from affecting your tests? You know, that's not a good thing?"

    That most probably depends on what you are actually testing.
     
  10. Offline

    PandazNWafflez

    raGan. I think this is a little out of context. Optimizations should be allowed to happen in this situation because it's just part of the process.

    I also said that reflection 'can' turn out faster (after optimization), in limited situations, not that it is faster always.

    Finally, I didn't explain the entire of how JVM / JIT works because there was no point. I didn't attempt to say that the reflection version of OP'S tool is better, I attempted to prove that it can be useful if you don't want to depend on CB because it isn't that slow.
     
  11. Offline

    MylesIsCool

    http://stackoverflow.com/questions/435553/java-reflection-performance

    Although you are trying to pop someones bubbles perhaps you should show the results you've produces, list your specifications.
    You waffle on about other JVM things which aren't relevant to the context you are talking about, it's verified by a lot of developers (as posted above in the link) that reflection in most cases can be slower. Now you can argue it is faster in version x, but most people are using the same standards the guys testing did which is what you would have to go with.



    Anyway on-topic:

    Would it not be suitable filing a bug report or looking for one to up-vote for incorrect orientations, or even after finding it creating a pull request?
    As much as I love reflection to it's glory this should be an inbuilt feature which works properly but anyway nice work @Guichaguri
     
  12. Offline

    raGan.

    That's something I agree with. It wasn't apparent to me from your post, though.
     
  13. Offline

    Garris0n

    Didn't you just encourage using reflection to avoid a CB dependency?
     
  14. Offline

    PandazNWafflez

    Garris0n Did you not read my post properly? I didn't say that you shouldn't use reflection on a server, I said you shouldn't use sun packages, including sun.reflect. java.lang.reflect is fine.

    MylesIsCool you should really read past the first answer in that topic - it isn't the best one by any means. In fact, the only reason that the examples (non-reflection) given was so fast was because the JVM could prove nothing was done. It more than likely didn't even run the for loop in the non-reflection examples. It didn't do this in the reflection examples. Read the comments on the answer.

    Reading further down the answers, you get to somebody who used arrays to make it a fair test, and for him the difference between reflection and regular was 150ns. A TINY difference, not even 1ms. Someone in the comments of that answer ran it and got a difference of 5ns...

    EDIT by Moderator: merged posts, please use the edit button instead of double posting.
     
    Last edited by a moderator: Jun 9, 2016
  15. Offline

    Garris0n

    You said you shouldn't use sun packages because they could be removed or changed, but you also recommended using reflection to avoid a CB dependency, which is bad for similar reasons (really, it's worse).
     
  16. Offline

    raGan.

    We could also say that relying on implementation is bad. Using reflection does not necessarily have to be worse, it's more of a trade-off, depending on your needs.
     
  17. Offline

    Garris0n

    Yes, but using reflection to be version independent is a bad idea. Imagine, say, there was an "a()" method that broke a block. You used reflection to call a() on a block in any version. Then they switch a() to blow up a block. Now your plugin blows up blocks, which is obviously bad.

    You should always have your NMS break on a version it's not written for.
     
  18. Offline

    PandazNWafflez

    Garris0n You've completely missed the point. I didn't say that it was better to use reflection so that you avoid depending on CraftBukkit, I said that some people don't want to build on CraftBukkit and so can use the reflection version if they wish. It's entirely personal preference.
     
  19. Offline

    Garris0n

    It's not just personal preference, using reflection to avoid building against CB is a bad idea. Well, unless you include a fail-safe that checks if the version is supported. This is the way to do it should you want to support multiple versions.
     
  20. Offline

    PandazNWafflez

    Garris0n No, using reflection means you can easily support multiple versions by only changing a couple of variables.
    Code:java
    1. String methodName = null;
    2. String className = null;
    3. switch (Bukkit.getMinecraftVersion() /* This isn't the method name probably */) {
    4. case "1.7.10":
    5. methodName = "a";
    6. className = "net.minecraft.server_R1_7_10";
    7. break;
    8. // ETC
    9. }
    10.  
    11. // reflection


    While it is 'safer' to do the thing in the thread you linked, it's really over the top when your plugin isn't very big, and this is much easier.
     
  21. Offline

    raGan.

    That's true, I was thinking more about CB, which doesn't change that much, but dynamic package names just make it break nonetheless.
     
  22. Offline

    Garris0n

    The dynamic packages are good, that's what prevents what I said above. Using reflection to circumvent them creates the problem all over again.

    Also, "doesn't change that much"? Look at the Git diff for the 1.7.2 update...
     
  23. Offline

    raGan.

    NMS changes are often bigger. But minor minecraft version changes don't affect CB that much. (usually)

    Who am I even trying to convince here, yes, it is always a bad idea if there's a risk of breaking something. Don't do drugs kids.
     
Thread Status:
Not open for further replies.

Share This Page