Improves item comparison performance #3258

WalshyDev · 2021-09-15T10:30:15Z

Description

There are a lot of situations where SF items are CraftItemStacks not SlimefunItemStacks

This is most obvious with cargo and has been a big bottleneck there (~60%).

This PR addresses the cargo-specific point but will probably carry over to other parts of the code here and in addons.

In Cargo we wrap the items being used for comparison, these items can still be SF items just, they look like regular Bukkit items. So, the is item similar check should account for this.

With this change, it will check if it's wrapped and if it has item meta, if it does then it will try and get IDs to compare. Otherwise, fall back to the usual item meta comparison. This improves cargo performance by about 60% when filtering SF items.

The reason this change has such a big impact is simple, the display name code is terrible. It will decode JSON and create message components just for getting the name. It's very awkward and not nice to use. The ItemMeta#getDisplayName alone accounts for 60% of cargos #insert processing time in the numerous Spark reports we have received.

Call stack for this:

Proposed changes

Check if an item is an ItemStackWrapper and try to compare IDs there too. It will only do it currently if both items are SlimefunItemStacks but that isn't the case a lot of the time.

Related Issues (if applicable)

N/A

Checklist

I have fully tested the proposed changes and promise that they will not break everything into chaos.
I have also tested the proposed changes in combination with various popular addons and can confirm my changes do not break them.
I followed the existing code standards and didn't mess up the formatting.
I did my best to add documentation to any public classes or methods I added.
I have added Nonnull and Nullable annotations to my methods to indicate their behaviour for null values
I added sufficient Unit Tests to cover my code.

Requesting a review from @md5sha256 who initially introduced this wrapper code

There are a lot of situations where SF items are CraftItemStacks not SlimefunItemStacks This is most obvious with cargo and has been a big bottleneck there (~60%). This PR addresses the cargo specific point but will probably carry over to other parts of the code here and in addons. In Cargo we wrap the items being used for comparison, these items can still be SF items just, they look like regular Bukkit items. So, the is item similar check should account for this. With this change it will check if it's wrapped and if it has item meta, if it does then it will try and get IDs to compare. Otherwise, fall back to usual item meta comparison. This improves cargo performance by about 60% when filtering SF items.

github-actions · 2021-09-15T10:30:43Z

Your Pull Request was automatically labelled as: "💡 Performance Optimization"
Thank you for contributing to this project! ❤️

sonarcloud · 2021-09-15T10:33:06Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

66.7% Coverage
0.0% Duplication

md5sha256

The optimisation makes a lot of sense; LGTM.

svr333

Looks good to me

WalshyDev · 2021-09-16T12:53:03Z

With the help of @Sefiraat I have done some timings on a HUGE base, here are the results:

Non optimized:

[14:44:08 INFO]: ===== Slimefun Lag Profiler =====
[14:44:08 INFO]: Total time: 75.05ms
[14:44:08 INFO]: Running every: 0.5s (10 ticks)
[14:44:08 INFO]: Performance: :::::::::::::::::::: - Severe (75%)
[14:44:08 INFO]: 
[14:44:08 INFO]: 1434 blocks
  CARGO_NODE_INPUT - 400x (65.2ms | avg: 0.16ms)

Optimized:

[14:49:26 INFO]: ===== Slimefun Lag Profiler =====
[14:49:26 INFO]: Total time: 39.2ms
[14:49:26 INFO]: Running every: 0.5s (10 ticks)
[14:49:26 INFO]: Performance: :::::::::::::::::::: - Moderate (39%)
[14:49:26 INFO]: 
[14:49:26 INFO]: 1434 blocks
  CARGO_NODE_INPUT - 400x (30.43ms | avg: 0.08ms)

50% reduction!!!

J3fftw1 · 2021-09-16T12:53:44Z

With the help of @Sefiraat I have done some timings on a HUGE base, here are the results:

Non optimized:

[14:44:08 INFO]: ===== Slimefun Lag Profiler =====
[14:44:08 INFO]: Total time: 75.05ms
[14:44:08 INFO]: Running every: 0.5s (10 ticks)
[14:44:08 INFO]: Performance: :::::::::::::::::::: - Severe (75%)
[14:44:08 INFO]: 
[14:44:08 INFO]: 1434 blocks
  CARGO_NODE_INPUT - 400x (65.2ms | avg: 0.16ms)

Optimized:

[14:49:26 INFO]: ===== Slimefun Lag Profiler =====
[14:49:26 INFO]: Total time: 39.2ms
[14:49:26 INFO]: Running every: 0.5s (10 ticks)
[14:49:26 INFO]: Performance: :::::::::::::::::::: - Moderate (39%)
[14:49:26 INFO]: 
[14:49:26 INFO]: 1434 blocks
  CARGO_NODE_INPUT - 400x (30.43ms | avg: 0.08ms)

50% reduction!!!

Not even crediting me for running these test bish

Sefiraat · 2021-09-16T12:54:32Z

With the help of @Sefiraat I have done some timings on a HUGE base, here are the results:

Non optimized:

[14:44:08 INFO]: ===== Slimefun Lag Profiler =====
[14:44:08 INFO]: Total time: 75.05ms
[14:44:08 INFO]: Running every: 0.5s (10 ticks)
[14:44:08 INFO]: Performance: :::::::::::::::::::: - Severe (75%)
[14:44:08 INFO]: 
[14:44:08 INFO]: 1434 blocks
  CARGO_NODE_INPUT - 400x (65.2ms | avg: 0.16ms)

Optimized:

[14:49:26 INFO]: ===== Slimefun Lag Profiler =====
[14:49:26 INFO]: Total time: 39.2ms
[14:49:26 INFO]: Running every: 0.5s (10 ticks)
[14:49:26 INFO]: Performance: :::::::::::::::::::: - Moderate (39%)
[14:49:26 INFO]: 
[14:49:26 INFO]: 1434 blocks
  CARGO_NODE_INPUT - 400x (30.43ms | avg: 0.08ms)

50% reduction!!!

Love it.... the owners of this server will, in advance, thank you greatly for this when it's all approved xD

TheBusyBiscuit

LGTM, I will merge this on the weekend.

Also, we really should focus on writing very extensive unit tests for this method soon.
It is so widely used, yet not even remotely covered by tests.

src/main/java/io/github/thebusybiscuit/slimefun4/utils/SlimefunUtils.java

TheBusyBiscuit · 2021-09-19T13:42:14Z

src/main/java/io/github/thebusybiscuit/slimefun4/utils/SlimefunUtils.java

+                // Slimefun items may be ItemStackWrapper's in the context of cargo so let's try to do
+                // an ID comparison before meta comparison


multiline comment 👀

Suggested change

// Slimefun items may be ItemStackWrapper's in the context of cargo so let's try to do

// an ID comparison before meta comparison

/*

* Slimefun items may be ItemStackWrapper's in the context of cargo so let's try to do

* an ID comparison before meta comparison

*/

TheBusyBiscuit · 2021-09-19T13:45:55Z

src/main/java/io/github/thebusybiscuit/slimefun4/utils/SlimefunUtils.java

+                Optional<String> possibleSfItemId = Slimefun.getItemDataService().getItemData(possibleSfItemMeta);
+                Optional<String> itemId = Slimefun.getItemDataService().getItemData(itemMeta);
+
+                if (possibleSfItemId.isPresent() && itemId.isPresent()) {
+                    return possibleSfItemId.get().equals(itemId.get());
+                } else {
+                    return equalsItemMeta(itemMeta, possibleSfItemMeta, checkLore);
+                }


Now that I have time to look at this a little closer, I spot a way we can optimize this further.
If possibleSfItemMeta has no id, we don't need to extract the id from itemMeta, reducing it by one avoidable call.

Not the prettiest but you get the idea

Suggested change

Optional<String> possibleSfItemId = Slimefun.getItemDataService().getItemData(possibleSfItemMeta);

Optional<String> itemId = Slimefun.getItemDataService().getItemData(itemMeta);

if (possibleSfItemId.isPresent() && itemId.isPresent()) {

return possibleSfItemId.get().equals(itemId.get());

} else {

return equalsItemMeta(itemMeta, possibleSfItemMeta, checkLore);

}

Optional<String> possibleSfItemId = Slimefun.getItemDataService().getItemData(possibleSfItemMeta);

if (possibleSfItemId.isPresent()) {

Optional<String> itemId = Slimefun.getItemDataService().getItemData(itemMeta);

if (itemId.isPresent()) {

return possibleSfItemId.get().equals(itemId.get());

}

}

return equalsItemMeta(itemMeta, possibleSfItemMeta, checkLore);

Scratch the above.
To declutter this:

We should implement a CustomItemDataService#compare(...) method.

I'll make a follow-up commit.

yeet

WalshyDev added the 💡 Performance Optimization This pull request improves performance. label Sep 15, 2021

WalshyDev requested a review from a team as a code owner September 15, 2021 10:30

md5sha256 approved these changes Sep 15, 2021

View reviewed changes

Sfiguz7 approved these changes Sep 15, 2021

View reviewed changes

TheBusyBiscuit assigned TheBusyBiscuit and Sfiguz7 Sep 15, 2021

svr333 self-assigned this Sep 16, 2021

svr333 approved these changes Sep 16, 2021

View reviewed changes

TheBusyBiscuit approved these changes Sep 16, 2021

View reviewed changes

src/main/java/io/github/thebusybiscuit/slimefun4/utils/SlimefunUtils.java Show resolved Hide resolved

TheBusyBiscuit previously requested changes Sep 19, 2021

View reviewed changes

TheBusyBiscuit merged commit 4727b2a into master Sep 19, 2021

TheBusyBiscuit deleted the performance/improve-item-comparison-performance branch September 19, 2021 16:58

variananora mentioned this pull request Dec 21, 2021

Cargo ignores lore of slimefun items, causing backpacks to stack. #3379

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improves item comparison performance #3258

Improves item comparison performance #3258

WalshyDev commented Sep 15, 2021

github-actions bot commented Sep 15, 2021

sonarcloud bot commented Sep 15, 2021

md5sha256 left a comment •

edited

Loading

svr333 left a comment

WalshyDev commented Sep 16, 2021

J3fftw1 commented Sep 16, 2021

Sefiraat commented Sep 16, 2021

TheBusyBiscuit left a comment

TheBusyBiscuit Sep 19, 2021

TheBusyBiscuit Sep 19, 2021 •

edited

Loading

TheBusyBiscuit Sep 19, 2021

TheBusyBiscuit Sep 19, 2021

		// Slimefun items may be ItemStackWrapper's in the context of cargo so let's try to do
		// an ID comparison before meta comparison

Improves item comparison performance #3258

Improves item comparison performance #3258

Conversation

WalshyDev commented Sep 15, 2021

Description

Proposed changes

Related Issues (if applicable)

Checklist

github-actions bot commented Sep 15, 2021

sonarcloud bot commented Sep 15, 2021

md5sha256 left a comment • edited Loading

Choose a reason for hiding this comment

svr333 left a comment

Choose a reason for hiding this comment

WalshyDev commented Sep 16, 2021

J3fftw1 commented Sep 16, 2021

Sefiraat commented Sep 16, 2021

TheBusyBiscuit left a comment

Choose a reason for hiding this comment

TheBusyBiscuit Sep 19, 2021

Choose a reason for hiding this comment

TheBusyBiscuit Sep 19, 2021 • edited Loading

Choose a reason for hiding this comment

TheBusyBiscuit Sep 19, 2021

Choose a reason for hiding this comment

TheBusyBiscuit Sep 19, 2021

Choose a reason for hiding this comment

md5sha256 left a comment •

edited

Loading

TheBusyBiscuit Sep 19, 2021 •

edited

Loading