search

Tradeshop / TradeShop

Unique, new, powerful Minecraft TradeShop plugin!
https://tradeshop.github.io
Apache License 2.0
25 stars 11 forks source link

Added Caching for DataStorage Load/Saves #125

Open KillerOfPie opened 2 years ago

KillerOfPie commented 2 years ago

Pull Request Etiquette

There are several guidelines you should follow in order for your Pull Request to be merged. Replace [ ] with [x] to cross the checkbox.

It is sometimes better to include more changes in a single commit. If you find yourself having an overwhelming amount of commits, you can rebase your branch.

Changes

Replace this sentence with a description of che changes introduced by this PR.

KillerOfPie commented 2 years ago

Testing Comment from 2.5.1 PR. Made there since I had been typing it our while testing and didn't want to loose screenshots from moving here...

https://github.com/Tradeshop/TradeShop/pull/124#issuecomment-1084092248

SparklingComet commented 2 years ago

Replying to https://github.com/Tradeshop/TradeShop/pull/124#issuecomment-1084092248

  1. I am not familiar with the exact meaning of those graphs, what do the red lines mean?
  2. When does the sharp increase in the test of the cached trades happen? (Check whether it Is when you start trading?
  3. Do we know what those spikes in the hopper tests are caused by? Is that the hopper cache being filled and emptied at regular intervals? We might want to confirm by roughly timing the period there.

If the answer to all of those questions is "yes", we might be looking at something good.

KillerOfPie commented 2 years ago

Realized I didn't have a comparison without the plugin running...

The Back may have happened before the marker at the top of the last peak(since it cuts out before gc normally clears it)...

image Pre-Teleport Avg mspT: forgot to check Post-Teleport Avg mspT: ~60ms Post-Teleport Peak mspT: ~85ms Return Avg mspT: ~3ms

KillerOfPie commented 2 years ago

Replying to #124 (comment)

  1. I am not familiar with the exact meaning of those graphs, what do the red lines mean?
  2. When does the sharp increase in the test of the cached trades happen? (Check whether it Is when you start trading?
  3. Do we know what those spikes in the hopper tests are caused by? Is that the hopper cache being filled and emptied at regular intervals? We might want to confirm by roughly timing the period there.

If the answer to all of those questions is "yes", we might be looking at something good.

  1. The red lines were added by me to indicate the rough start and end of the test I was doing and the graph is just memory usage. With 5GB of memory each vertical line indicates 1.125GB of memory and most of the time i tried to highlight as close to the peak as possible which is the pop-up that shows the used amount. Unfortunately mspT isn't graphed so that is read manually.
  2. All of the shops would have been added to the cache as soon as I load into the server since I was the owner of them, unfortunately I didn't get the startup memory usage in the initial graph but I don't think it was that much higher. Same with the initial Peak in mspT, didn't watch but i don't think it was unreasonable
  3. As you can see in the reference above without the plugin those peaks are from standard gc in mc. As long as I am in the testing area none of the hoppers should leave the cache since they are only removed if not access for more than 1sec. The shops and linkage stay in much longer which concerned me this morning considering the server was using almost 4GB of memory but after the above reference it seems that they are negligible.
KillerOfPie commented 2 years ago

Going to leave this out of 2.5.1 since I want to get that out sooner and maybe add in 2.5.2 or 2.6.0. Right now this does need to be changed to use a combination of the cached items and the old checking method which I want to remove. The only Alternative option I can think of now would be to keep all shop locations and linkage data in memory and have them temporarily cached like now. This would make all checks for finding shops a lot quicker and should be less memory intensive then the hoppers caching just with a constant cost.

I think I would store in a List for Shops and linkage data would be a Map<Location storage block, Location linkedShop>. These would either be in;

The ShopCache and PlayerCache would staysince those would still be useful.

@SparklingComet input?

SparklingComet commented 2 years ago

I'd say let's go with DataCache.

This might actually be a viable alternative to the current hopper cache, or at least a unification so that we don't have several caching systems being flung around.

We'd probably still do well to only cache shop data for currently loaded chunks (loading all shop data into memory is a big no-no). Since chunks are being loaded/unloaded by the hundreds every second on an active server, we might want to have a mechanism that discards unloaded chunk data every X mins. This might still be an issue though, as many chunks are being loaded every second, so that we would still have a significant amount of filesystem interactions for potentially unused shops...

It would be good to do some maths analogous to the one I started for the hopper cache in order to know what kinds of memory footprint to expect.

SparklingComet commented 2 years ago

This is actually bitter because had we been using SQLite storage all along, this might have been significantly less complicated.

KillerOfPie commented 2 years ago

s. This might still be an issue tho

Hoppers and shops will stay with the time based caching based on last access since I think this is the most efficient for both. Shops being in a loaded chunk does not mean they are likely to be loaded from file but them being loaded once means they are much more likely to be accessed again(I think at least) so caching on access is much better(imo). As long as hoppers are loaded they will be hitting the cache so the current timed-cache works well for that(may bump up the time-out on them since I think I left it at 1 sec but if the hopper settings are changed they more likely to fall out before getting processed again)

So Agree to Add, but in 2.5.1(and push back the fairly major bug fix another week) or 2.5.2(since it still isn't really a feature add, or 2.6.0 with whatever changes we make there?)

This is actually bitter because had we been using SQLite storage all along, this might have been significantly less complicated.

Agreed, still think we need to put this in for either 2.6 or 2.7 so that it has time to mature a bit before 3.0. All the backend should be fairly flushed out so adding it shouldn't be too "difficult" ( should just be adding in the switches and basic code in DataManager and any needed files for the connections and getting/setting)

KillerOfPie commented 2 years ago

Previous Timing for the caching branch:

Pre-Teleport Avg mspT: ~3.3ms Post-Teleport Avg mspT: ~78ms Post-Teleport Peak mspT: ~150ms Return Avg mspT: ~2.8ms

Timing with new changes:

Pre-Teleport Avg mspT: ~3.3ms Post-Teleport Avg mspT: ~78ms Post-Teleport Peak mspT: ~90ms Return Avg mspT: ~2.8ms

Just wanted to point out that this change dropped the peak TPS on the hopper test by 40% and I'm not seeing any noticable changes in memory

KillerOfPie commented 2 years ago

Seems the peak TPS frop the test was lower because of some bugs up to ~105mspT peak and settles around ~80mspT now. But it has gone through minor testing.

KillerOfPie commented 2 years ago

I think changing over to using just file storage and caching will allow use to be independent of MC versions however, in order to do this properly I would want to remove all the legacy updating code. This is going to potentially break shops made before 2.3(when we added file storage I think?) and definitely break any hopper protections we had before 2.3 or 2.4 depending on when I fixed the Chest > shop Linkage data. Even with the update code we could only update shops that were interacted with meaning some old shops may exist on servers.

This change will have to happen eventually since the legacy code does interfere with the new code being faster(In a lot of cases I can check the caches but need to fall back to persistent data for block names to make sure we catch old blocks)

Questions

  1. Is being version Independent worth losing our own back support
  2. Should we make the cross-over in 2.6 and just hold this pr for then since this is where I started to notice the added processing?
  3. Should we post a question on discord for 1?
SparklingComet commented 2 years ago

Sorry for the late reply.

So Agree to Add, but in 2.5.1(and push back the fairly major bug fix another week) or 2.5.2(since it still isn't really a feature add, or 2.6.0 with whatever changes we make there?)

I'd say push to the release v2.5.1 with the bug fix and consider the caching carefully, making it v2.5.2 (let's not hurry unnecessarily). I'd still prefer doing it before b2.6.0 in order to test separately.

Agreed, still think we need to put this in for either 2.6 or 2.7 so that it has time to mature a bit before 3.0. All the backend should be fairly flushed out so adding it shouldn't be too "difficult" ( should just be adding in the switches and basic code in DataManager and any needed files for the connections and getting/setting)

Yes, I think this is a good plan. We'll introduce it first as an "experimental" feature. Eventually (after a reasonable time during which enough testing was done) we will make SQLite the default storage option.

SparklingComet commented 2 years ago
  1. Is being version Independent worth losing our own back support

Why will this be version independent? Are we not using newer Bukkit API features that are not supported by older versions beside persistent data containers etc.?

  1. Should we make the cross-over in 2.6 and just hold this pr for then since this is where I started to notice the added processing?

I'd still rather introduce caching in v2.5.2 so that reasonable testing was done. If I understood your proposal correctly, the crossover could ideally be implemented as an extension of the currently implemented caching system, at another time (v.2.6.0).

I'm still not convinced about losing out on our "back support". v2.3.0 is way too recent for that...

  1. Should we post a question on discord for 1?

I don't know if people will understand the implications of this, but we can certainly try. Maybe we could make a new channel in order for people to discuss this change.

KillerOfPie commented 2 years ago
  1. Is being version Independent worth losing our own back support

Why will this be version independent? Are we not using newer Bukkit API features that are not supported by older versions beside persistent data containers etc.?

If I'm not mistaken Persistent data is the only thing that isn't locked behind a version gate, since it is required for shops to use it vs things like shulker boxes, bags, and signs that were implemented over time but don't cause problems unless we try to use them(these all have version checks before they are used unless I missed some of them, and Signs have an enum that helps decide which signs to use based on the version)

  1. Should we make the cross-over in 2.6 and just hold this pr for then since this is where I started to notice the added processing?

I'd still rather introduce caching in v2.5.2 so that reasonable testing was done. If I understood your proposal correctly, the crossover could ideally be implemented as an extension of the currently implemented caching system, at another time (v.2.6.0).

I'm still not convinced about losing out on our "back support". v2.3.0 is way too recent for that...

As long as the shops were interacted with between 2.3 and before we make the change everything should work perfectly, so people running before that may need to make a mid upgrade, interact with all the shops, then upgrade to the newer version(I don't think we can automate this since we would have to search through every block in the world and run checks on each)

The worst case people would just have to break and re-create their shops.

  1. Should we post a question on discord for 1?

I don't know if people will understand the implications of this, but we can certainly try. Maybe we could make a new channel in order for people to discuss this change.

I think what we want their input on would be a fairly simple question?

We are thinking about making changes that may cause issues when upgrading from TradeShop version before 2.3, these changes should all the plugin to be used on any minecraft version as well as potentially providing more stability and improved performance.

Is the potential loss of/force recreation of shops worth the improvements for your server?

SparklingComet commented 2 years ago

If I'm not mistaken Persistent data is the only thing that isn't locked behind a version gate

The persistence API dates to 2019. We dropped several MC versions back when we started making use of it...

As long as the shops were interacted with between 2.3 and before we make the change everything should work perfectly, so people running before that may need to make a mid upgrade, interact with all the shops, then upgrade to the newer version(I don't think we can automate this since we would have to search through every block in the world and run checks on each)

In that case it should actually be fine, however this needs to be tested well... It would be bad for people to lose their shop data, even though they'd be supposed to keep a backup.

The worst case people would just have to break and re-create their shops.

I wouldn't say that, we would not want players who are too slow to do so having their shops looted or destroyed.

SparklingComet commented 2 years ago

I think what we want their input on would be a fairly simple question?

I suppose so, yes. Hopefully people will actually chime in...