1.16.5 Is it practical to cache data from yaml files?

Discussion in 'Spigot Plugin Development' started by Corals, Mar 28, 2021.

  1. Currently, my code doesn't have any caching for the values taken from config.yml or any other configuration files I have, as opposed to setting them in a final variable at plugin startup. When the value is needed, I just call it directly from the file itself. Is it practical to read a file all the time? Does it have any performance impact? Is setting them up to variables beforehand and getting from that instead of always reading the file better? I'd like to do something about this before I go further down the hole and making it hard to do optimizations in the future. Thanks!
     
  2. If you're using Bukkits default FileConfiguration the values are already loaded and cached into a Map. When calling getString for example it's not performing any IO, which is why "reloading" config files are necessary.

    Setting them to variables technically is a performance boost but judging by your use case I highly doubt there's a difference between that and a Map lookup. Up to your code design at that point
     
    • Agree Agree x 2
  3. Ohh I see.. Yes, I'm only using FileConfiguration and there's not much data that I need to read.. Thanks for answering!
     
  4. I suggest you to create a plugin configuration class. That will be more performant and much more convenient

    For example:

    Code (Java):
    package whatever.your.package;

    import org.bukkit.configuration.file.FileConfiguration;
    import org.bukkit.configuration.file.YamlConfiguration;

    import java.io.File;

    public class PluginConfig {

        // property paths in your config file

        private static final String VALUE_A = "a";

        private static final String VALUE_B = "b";

        // values

        private int valueA;

        private String valueB;

        // make constructor private
        private PluginConfig() {
        }

        // getters

        public int getA() {
            return valueA;
        }

        public String getB() {
            return valueB;
        }

        // factory method
        public static PluginConfig newInstance(File file) {
            PluginConfig config = new PluginConfig();
            FileConfiguration fConf = YamlConfiguration.loadConfiguration(file);
            config.valueA = fConf.getInt(VALUE_A);
            config.valueB = fConf.getString(VALUE_B);
            return config;
        }

    }
     
    Then create an instance in your main class and access it whenever you want
     
    #4 mrko900, Mar 28, 2021
    Last edited: Mar 29, 2021
    • Informative Informative x 2
  5. Ohhh yeah, that is more convenient and understandable. Thanks for the idea, I'd gladly use it :D
     
  6. That’s a bad way to initialize an object. If you access your config a lot you’re going to be ending up with a lot of objects and potentially lead to a memory leak.
     
  7. Um lol, I'm gonna create only one instance, for example, in onEnable method and then refer to it but ofcourse not instantiating the class every time
     
  8. Either way that’s still bad OOP practice. If you were building a much larger project and had multiple people working on it that leaves the perfect opportunity for a memory leak to happen by doing PluginConfig.getInstance()....


    EDIT:
    Move all of your class initialization into the constructor and change your getInstance() to this.


    Code (Java):
    public static PluginConfig getInstance() {
            if (instance == null)
                instance = new PluginConfig();
            return instance;
        }
    This insures that only one instance of the class can we exist.

    EDIT 2:
    Make sure the class constructor is private, otherwise it beats the entire purpose.
     
    #8 Justin393, Mar 28, 2021
    Last edited: Mar 28, 2021
  9. What exactly do you mean?
     
  10. Read my edit
     
  11. I don't think making PluginConfig a singleton is a good idea because config can change during the runtime (that's a really rare situation but it is possible).
     
  12. Doesn’t matter. FileConfigration provided by Bukkit loads all config values into a Map at enable. You would have to reload the implemented config to get the changes anyway.
     
  13. I actually know that, and honestly, I don't understand what exactly do you want to say
     
  14. That doing it your way is bad practice and that it can lead to memory leaks.
     
  15. Strahan

    Benefactor

    How is that any more performant or convenient? It won't perform any better, and as far as convenience it's way more of a PITA (at least initially) as you have to lay out the whole schema manually.
     
  16. Not that there aren’t cons to this approach, but it does matter somewhat if you have deeply nested YAML sections. I do also like assigning a concrete type and name in terms of accessing configuration items, so that is a bit more “convenient” than accessing by String literal keys, especially if it gets accessed more than once or if it is mapping something to a type.

    It’s not going to cause memory leaks. I prefer to have a singleton configuration myself, but there is no reason a factory method isn’t valid for this purpose.
     
  17. It is extremely convenient to use if complex objects are stored in the config, and more performant than creating FileConfiguration instance every time you need to access config file, that's what I meant.
    Maybe I am dumb, or blind, or something but I still can't clearly understand what do you mean
     
  18. Anytime PluginConfig.getInstance() is called it's creating a new instance of the class. It will cause a memory leak overtime if it keeps being called. I would have to see the exact implementation he's doing, but it's possible that the objects would never be released to the garbage collector because something is holding reference to them.
     
  19. Let's say you and I are working on the plugin together. I want to call the config and see you have a nice handy PluginConfig helper class. So I do PluginConfig.getInstance().getValue() Whoops, there's another object floating around.
     
  20. You can apply that logic to any class constructor or factory method and conclude that it will “lead to a memory leak.” Obviously, a memory leak can occur if references are not balanced with dereferences.

    Regardless, using a singleton to fix a memory leak seems to fix a problem that doesn’t exist. Where are the references to a configuration going to come from? What practical scenario would warrant you to hold on to a reference of the configuration such that it would cause a memory leak?

    Even
    if you are getting a memory leak from using a factory method, the memory leak is an external problem. This means that 1) you should fix the memory leak itself rather than attempting to get around the issue with a singleton and 2) there are reasons to believe that a singleton will not fix a memory leak (e.g. if it is being stored as a value in a Map).

    Whatever new instance created through this factory method will go out of scope at the end of the function call through this method if you do not keep it in whatever value is returned by getValue() or any internal methods. In fact, there is reason to think that a compiler optimizing for exit analysis enabled would not even allocate the configuration on the heap if the function is sufficiently short, so I’m not sure where you are getting this memory leak stuff from.

    Again, to clarify, I am all for using a singleton to store your configuration. However, I disagree that the approach suggested by another user would lead to memory leaks or is bad practice.
     
    • Agree Agree x 1