Fix ItemStack.EMPTY.getItem() rarely returning null due to threading

[Technici4n]

See for example AztechMC/Modern-Industrialization#404.

[modmuss50]

Does this happen in vanilla?

[Technici4n]

Unclear whether it happens in vanilla or not, but with mods it creates random crashes that don't point to the original modification. Transfer API is also guilty of calling .setCount(0) on empty stacks. You'd expect this to be a safe operation, and patching all the call sites is much uglier...

As for the performance impact: it seems like the whole injector gets inlined into updateEmptyState and the CI allocation is gone... Not too sure as I'm not used to reading hotspot output and had to do some "manual" disassembly, but it looks ok.

[shartte]

Unclear whether it happens in vanilla or not, but with mods it creates random crashes that don't point to the original modification. Transfer API is also guilty of calling .setCount(0) on empty stacks. You'd expect this to be a safe operation, and patching all the call sites is much uglier...

As for the performance impact: it seems like the whole injector gets inlined into updateEmptyState and the CI allocation is gone... Not too sure as I'm not used to reading hotspot output and had to do some "manual" disassembly, but it looks ok.

How did you get the disassembly?

There is a tool that simplifies it greatly but I forgot the details (as usual :-P))

[Technici4n]

I used -XX:+PrintAssembly which gave this:

No decompiler module to directly emit readable assembly, but I copy/pasted the instructions manually into https://defuse.ca/online-x86-assembler.htm. 😅

I also checked the inlining log (with -XX:+PrintInlining):

which suggests that the method got inlined.

FYI this is what I used to run the tests:

Not perfect, but seems OK at a glance.

[liach]

Does this not hurt other items set to count 0 because they won't return null but a wrong value, which is less severe?

I personally recommend patching the instance field write/reads to local variables or mark the method synchronized for a more comprehensive solution.

[Technici4n]

We could overwrite the method to this.empty = this.item == null || this.item == Items.AIR || this.count <= 0.

[Technici4n]

Changed to an overwrite. Using a cancellable injector because of lithium's mixin: https://github.com/CaffeineMC/lithium-fabric/blob/1.19.x/dev/src/main/java/me/jellysquid/mods/lithium/mixin/item/ItemStackMixin.java that will conflict with any sort of @Overwrite.

[emilyploszaj]

Does this happen in vanilla?

For the bug I've reported to mojang, I was able to reproduce this bug in vanilla after several hours, you can see the crash log here. https://bugs.mojang.com/browse/MC-258939

[modmuss50]

I was able to reproduce this bug in vanilla after several hours

Thats awesome! Thanks 👍

[Noaaan]

Does this happen in vanilla?

For the bug I've reported to mojang, I was able to reproduce this bug in vanilla after several hours, you can see the crash log here. https://bugs.mojang.com/browse/MC-258939

Wow. We theorized and joked about this potentially being a vanilla bug. To have it confirmed is another story. Have hunted this issue for over 6 months, so finally putting it to rest is great.
Noaaan/MythicMetals#94

[emilyploszaj]

Wow. We theorized and joked about this potentially being a vanilla bug. To have it confirmed is another story. Have hunted this issue for over 6 months, so finally putting it to rest is great. Noaaan/MythicMetals#94

Tech told me he didn't think this was a vanilla bug so I had to prove him wrong