Use BlockPositions internally instead of Locations

JustAHuman-xD commented 2 months ago

Description

BlockStorage & related classes currently use Locations internally instead of BlockPositions. Locations are comprised of a Reference, 3 doubles, and 2 floats, while BlockPositions are comprised of only a WeakReference, and a long. By switching to using BlockPositions we would save MASSIVELY on memory with the current implementation of BlockStorage.

Once this PR is ready to go and approved I will make a matching PR that updates all of Slimefun's internals to use the new methods instead of the deprecated old ones.

Proposed changes

(Very simple overview, will write a full blown one later)
Refactors BlockStorage, TickerTask, BlockMenu, BlockMenuPreset, SlimefunProfiler, ProfiledBlock, and ErrorReport to use BlockPositions instead of Locations
- A couple methods needed to be renamed due to their signature & to avoid breaking changes
- I still need to setup the javadocs for the new methods
- To avoid breaking changes all old methods still exist and just overflow to the new methods.
- I still need to update java docs and add deprecation warnings to all of these

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 have made sure that the proposed changes do not break compatibility across the supported Minecraft versions (1.16. - 1.20.).
[x] I followed the existing code standards and didn't mess up the formatting.
[x] I did my best to add documentation to any public classes or methods I added.
[x] 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.

github-actions[bot] commented 2 months ago

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

github-actions[bot] commented 2 months ago

Slimefun preview build

A Slimefun preview build is available for testing! Commit: fc8eae0d

https://preview-builds.walshy.dev/download/Slimefun/4220/fc8eae0d

Note: This is not a supported build and is only here for the purposes of testing. Do not run this on a live server and do not report bugs anywhere but this PR!

JustAHuman-xD commented 2 months ago

Potentially controversial things I want opinions on:

adding BlockStorage#getBlockInfo for the new versions of BlockStorage#getLocationInfo as most other methods call it BlockInfo
BlockStorage#getRawStorage & TickerTask#getLocations return empty maps/sets now, we could make it remap the data to use locations and return that but that would kinda defeat the point of the memory conservation effort being made with this PR, open to discussion ofc tho
Deprecating the old methods, I feel this is necessary since there would be no reason to not use the new methods & it would encourage using BlockPosition and not Location
Possibly other things but these are the main ones I can think of rn

JustAHuman-xD commented 2 months ago

Once this PR is ready/in a good state I'll make an additional PR that updates all of the slimefun implementations, tests, etc, that use the old methods to use the new ones

md5sha256 commented 2 months ago

Potentially controversial things I want opinions on:

adding BlockStorage#getBlockInfo for the new versions of BlockStorage#getLocationInfo as most other methods call it BlockInfo

BlockStorage#getRawStorage & TickerTask#getLocations return empty maps/sets now, we could make it remap the data to use locations and return that but that would kinda defeat the point of the memory conservation effort being made with this PR, open to discussion ofc tho

Deprecating the old methods, I feel this is necessary since there would be no reason to not use the new methods & it would encourage using BlockPosition and not Location

Possibly other things but these are the main ones I can think of rn

I'm pretty sure #getRawStorage and #getLocations are used so they absolutely cannot return empty values. I think we should deprecate them for the new methods but still have them return a (immutable?) copy.

JustAHuman-xD commented 2 months ago

I'm pretty sure #getRawStorage and #getLocations are used so they absolutely cannot return empty values. I think we should deprecate them for the new methods but still have them return a (immutable?) copy.

The only usages I could find for getrawstorage outside of base SF was headlimiter (which I can PR to fix) and my own project that isn't released.

As for getLocations I couldn't find any usages but I could obviously have missed some.

The thing with copying it is that it'll use WAY more memory than the old method did since every time the method is called itll create a NEW location instance for every block position. While previously the immutable map would have (I assume) just been references.

Sfiguz7 commented 1 month ago

Closure of shame incoming

JustAHuman-xD commented 1 month ago

me and Idra did major bad napkin math, the memory save is negligible #blame-idra

Slimefun / Slimefun4