Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve performance of chunk saving #64

Closed

Conversation

hohserg1
Copy link

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

performance improvement: calculate each value only one time at game start-up
should speed-up MixinBlockStateContainer#reid$newGetDataForNBT at stateIDMap
Common case: block with no properties. Single blockstate of such blocks have same hashCode at all.
jchung01

This comment was marked as outdated.

@jchung01
Copy link

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
Copy link
Author

hohserg1 commented Jul 11, 2024

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
Copy link

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.

@hohserg1 hohserg1 closed this Jul 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants