Solved Function cooldown

Discussion in 'Spigot Plugin Development' started by titivermeesch, Jul 29, 2018.

  1. Hi

    Ok so I don't have any code samples here, but let's say I have a function that is triggered by a button.
    What I want to do is when he clicked the button, a cooldown starts so he can't click on it anymore for x seconds.
    How could I do this.
     
  2. Store a Map<UUID, Long> which stores the current time in millis. When they click, if:

    time + delay >= Now: cancel
    time + delay < Now: remove it.
     
  3. Isn't there a more optimised way of doing this?
    Like an int that is counting down?
     
  4. An alternative with automatic cleanup could be to store a reference to the player (or their UUID) in a Set, and then schedule a delayed task to run after X ticks (where X is the cooldown) to remove the reference in the Set. Then share that Set with the listener that's checking to see if the player can use the button.
     
  5. What do you mean by optimized? Easier to read? Faster? Simpler?

    When you're working with a game loop, and you want things to happen in the game based on the progression of that game loop, you need to work with the tools that allow you to hook into that loop. The way to do that in Bukkit is by scheduling tasks or listening for events. You can't just write a for-loop and be done with it, if that's what you're thinking. It's unfortunately not that simple.
     
  6. Choco

    Moderator

    You're not going to get much faster than a hash lookup and a long comparison. Decrementing an integer involves a timed task which takes up more resources than necessary. Map<UUID, Long> is the way to go here.
     
  7. Might be worth mentioning that when you put something in a map, you should think of ways to remove that something again. The suggested approach on its own is a potential memory leak.

    As for performance, scheduling a delayed task isn't as heavy as you might think. It pretty much just involves a reference to a Runnable plus the memory required for the instance fields. Compared to all the stuff that's going on already, scheduling a task is negligible. Readable, correct code should have a higher priority than performance unless you're sitting in a tight loop. I would be surprised if cooldown implementations for button presses could bog the server down as much as you make it sound like it can.
     
  8. @garbagemule I don’t see how the map i proposed would lead to memory leaks? It’s storing a UUID not a Player. It gets removed if checked and time + delay < now
     
  9. Choco

    Moderator

    I did not say it took an absurd amount of memory. I said it takes up more resources than necessary. If you need to have per-player cooldowns, would you rather mapping a UUID (a 128-bit object) to a single, primitive 32-bit long, or mapping a UUID to an instance of a runnable. You're now looking at scheduling n amounts of runnables at once, not just one. As for the memory leaks, you should be cautious of clearing out any sort of object that holds data, maps included. You have the option of removing the user's UUID upon logout, or clearing onDisable.
     
  10. If you map players to "press time" (when the player pressed the button), then as long as a player has pressed the button once, the player will remain in the map. You say "remove it", but that's not actually what you're doing. You're storing a new "press time" every time. So eventually the map will be full of player references (doesn't matter that it's UUIDs, they still take up memory), until the server is restarted, at which point the map will be empty, of course. If this is not how your proposal works, then please feel free to elaborate. But going solely by what you said in your first post, there is a memory leak.

    "Necessary" is quite subjective here. Even UUIDs could be considered excessive in some ways. Oh, and your longs aren't 32-bit, they're 64-bit. Which is also quite a lot of information to carry around. But if you don't expect your server to be up for more than 68 years, you could use a 32-bit int. Heck, with a little bit of bit shifting magic and an exponential rather than linear approach to the values stored in it, you could probably get away with storing it as a byte. But of course, in that case, you're still storing quite a bit more than 8 bits, because that's just how Java's wrapper classes and generics work. Also, your code is completely unreadable.

    I think you should be careful about this sort of premature optimization. It's a rabbit hole you don't want to go down. It's a lot more helpful to know how to avoid memory leaks such as the one introduced here, than to shave a few bits off of the storage requirements in the underlying map. Even if a thousand users joined the server and used the button at the same time, the cooldown implementation with scheduling resources would not be the bottleneck.

    Let's do a bit of math. The storage requirement for the Runnable is actually only 64 bits plus whatever that Runnable holds (that's how object references in Java work). In this case you'd need a Player reference and a Set reference. Another two times 64 bits. So that's a total of 192 bits per active cooldown. With the Map approach, you need a Player reference and a Long reference, which both cost 64 bits. And then of course you need some way to remove the players from the maps - you said event listeners? At least 64 bits per listener for the reference, but it's actually a bit more since there's also a reference to the method itself and some other stuff in the RegisteredHandler down in the HandlerList. So at least 64 bits at all times, plus the 128 bits for the references in the Map, and voila, back to 192 bits :) I left out a few things in both cases, so I think it's a fair comparison. The point is that there are hidden costs everywhere, and you should only worry about them if they actually cause issues. Benchmark first, then decide.