Removed broken chunk key optimization, use longs instead #586
Conversation
| @@ -251,14 +250,14 @@ public LightningStrike strikeLightningEffect(Location loc, boolean isSilent) { | |||
| * Per-chunk spawn limits on various types of entities. | |||
| */ | |||
| private int monsterLimit, animalLimit, waterAnimalLimit, ambientLimit; | |||
| private Map<Integer, GlowStructure> structures; | |||
| private Map<Long, GlowStructure> structures; | |||
There was a problem hiding this comment.
Perhaps TLongObjectMap could be better here?
There was a problem hiding this comment.
You're talking about the trove stuff, correct? I hadn't seen it used anywhere else in the code base, and this is already pretty lean as it is. I try not to do too much premature optimization, and in this case I don't think we'll be storing millions of items in the map. In addition, while Trove is stable, it seems within the last couple years other libraries outperform it.
|
|
||
| /** | ||
| * The maximum height at which players may place blocks. | ||
| */ | ||
| private int maxBuildHeight; | ||
|
|
||
| private Set<Key> activeChunksSet = new HashSet<>(); | ||
| private Set<Long> activeChunksSet = new HashSet<>(); |
|
After much discussion, I scaled back the change so it only affected GlowChunk.ChunkKeyStore and GlowChunk.Key, preserving them rather than replacing them. We can observe how this affects memory/performance, and go from there. In addition, I had to make a tweak to GlowPlayer due to some changes introduced in Glowkit that prevented Glowstone from compiling. |
As discussed in discord, the chunk key class implemented its caching mechanism wrong. By doing (x * 31 + z), it was confusing the chunks at (1, 0) and (0, 31) for each other. This, I observed, lead to corruption in the saved chunks. I ported over Momo's use of longs for chunk keys, which in my testing is not observedly slower than the existing code.