search

TerraFirmaCraft-The-Final-Frontier / RoughlyEnoughIDs

Fork of JustEnoughIDs. Minecraft 1.13+ chunk format in 1.12.2, removing the block, item, & biome ID limits.
https://www.curseforge.com/minecraft/mc-mods/reid
MIT License
10 stars 11 forks source link

Improve performance of chunk saving #64

Closed hohserg1 closed 1 month ago

hohserg1 commented 1 month ago

We have some frequently used logic at https://github.com/TerraFirmaCraft-The-Final-Frontier/RoughlyEnoughIDs/blob/master/src/main/java/org/dimdev/jeid/mixin/core/world/MixinBlockStateContainer.java#L60-L64 that uses IBlockState as a key of HashMap, so this pr about caching of IBlockState#hashCode to not compute a same value multiple times. Also, the second commit aims to fix hashCode collisions by considering a block instance instead of only properties for hashCode of IBlockState

jchung01 commented 1 month ago

Actually, is it even necessary to generate the hashcode ourselves? Every state should be unique, so using the default hashCode() implementation of Object should be enough, and eliminates the expensive operation of hashing the map. VintageFix actually already does this, so the most we would need to do is copy their hashcode method in case the user isn't using VintageFix with REID.

hohserg1 commented 1 month ago

ty for review! yeah, bc of every blockstate is singleton, have sense to use default hashCode(). VintageFix are calling identityHashCode every time when hashCode required, but javadoc meaning it is not just returning value, its kinda calculated every time. image Also VintageFix are using this. Is it meaning this at @Overwrite is pointing to this of target class instance, but this at @Inject is pointing to mixin's this?

jchung01 commented 1 month ago

Yes, System#identityHashCode does do some calculation for the hash code, but relatively speaking, it is only "expensive" when calculating the hash code for the first time on an object - in this case, that's sometime during game load, which is fine. Following calls are cheap (see here). So I would say VintageFix's decision to use it is correct, and it is what we should use in this mixin. In fact, this PR/mixin may not be necessary, because this is a general optimization that VintageFix already does, and it seems unlikely a user would have REID but not VintageFix. However, if you want to copy VintageFix's hashCode impl to this mod for redundancy, go ahead and make the changes.

Also VintageFix are using this. Is it meaning this at @Overwrite is pointing to this of target class instance, but this at @Inject is pointing to mixin's this?

Normally, to reference the actual this of the mixin target's class, you would (in this case) cast like ((StateImplementation) (Object) this), but identityHashCode() just needs the Object, so any casting is redundant.