World exploration lag

[LemADEC]

As of prospects-1.0.5.jar, TPS lag spike are observed when players are exploring new chunks.
Our map is pregenerated so it's not world generation lag.
According to our profiling, it's related to the chunk scanning doing lookup with a slow hash computation, here:

prospects/src/main/java/com/dizzyd/prospects/world/OreDictCache.java

Line 79 in 5d6b06b

return oreNames.get(bs);

We do have the UndergroundBiomes mod which replaces vanilla stone with various alternative ones, hence the existing 'fast path' isn't used.

From a quick code review:

• BlockState.hashCode() is not cached and relatively inefficient to compute.
• BlockState.toString() will cause GC lag and relatively slow to compute.
• Block.hashCode() isn't defined, so it would use the object address. As such, it might be a better option, using a nested HashMap?
  or simply add support for ore dict stones (adding all blocks from ore:stoneAll to the fast path?).

[dizzyd]

Could you please share your tracing data, screenshots of stacks, etc?

[dizzyd]

Off the cuff, a few notes:

• The hashCode() is supposed to use the object address; that should be very quick.
• I'm not sure where BlockState.toString() would be called? Do you have a specific reference you're referring to?

I will take a look at using ore dict for stone to see if that helps.

[LemADEC]

I've seen a hashcode() lag pattern during profiling but it was negligible until I've realized it was showing up in the logs during lag spike events: https://gist.github.com/LemADEC/5a1d26de7a52fabe8104e6370904a7c9

I did a code review that showed inefficient hashCode() evaluation:
IBlockState.hashCode() is net.minecraft.block.state.BlockStateContainer.StateImplementation

public int hashCode()
{
    return this.properties.hashCode();
}

which is from com.google.common.collect.ImmutableMap<K, V>

public int hashCode() {
    return Sets.hashCodeImpl(entrySet());
}

(nota: entrySet() return value is cached)

which is from com.google.common.collect.Sets

static int hashCodeImpl(Set<?> s) {
    int hashCode = 0;
    for (Object o : s) {
        hashCode += o != null ? o.hashCode() : 0;

        hashCode = ~~hashCode;
        // Needed to deal with unusual integer overflow in GWT.
    }
    return hashCode;
}

which is a loop on all properties to call IProperty.hashCode() and do some maths.

IProperty.hashCode() is overridden in the case of Enum, Boolean and Integers, so almost everytime.

We're trying to grab a more focused profiling to confirm.

[LemADEC]

I was proposing BlockState.toString() as an alternative in your date structure (i.e. using the String representation for the HashMap key, instead of the BlockState itself).

[LemADEC]

Apparently, it happens when players are near the world border and generate new chunks outside of it:

nota: on above screenshot, the calls for OreDictCache.getOreName() totalizes 2900 ms out of 68500 ms.
There's other occurrences of calls to GameRegistry.generateWorld() in there, so it's only an underestimation of the total impact.

[dizzyd]

Alright, this is helpful. Let me do some digging and figure out a better way to translate a block state into a unique identifier.

[dizzyd]

So, I added some timing info and did some of my own testing:

• Generally, generation takes ~900-1000us with 1.0.5 of prospects, per chunk
• Adding UndergroundBiomes drives generation time up to ~3500-4000us
• I did some digging and found a different way of encoding state using integers; with this patch in, I'm seeing ~900us for 1.0.5 vanilla and ~1300us with UndergroundBiomes.

I think it's right, but would appreciate your help in validating that it doesn't break anything. Is there anyway I could give you a pre-build and have you re-profile with your setup?

[LemADEC]

just post an alpha version on curseforge, and we'll try it out.

[dizzyd]

Here's the current patch: https://gist.github.com/dizzyd/f02cd3f1103adcfd024f0c47a0f16ccd

I'll let you know once it's approved by CF.

[dizzyd]

Alpha available here: https://minecraft.curseforge.com/projects/prospects/files/2601810

[dizzyd]

@LemADEC how did it work?

[LemADEC]

Sorry, still working on that test.

[LemADEC]

Tested in single player: the updated version feels faster but the profiler is still unhappy.

Apparently, I've missed a GC lag caused by creating BlockPos for every blocks in the chunk, here:

prospects/src/main/java/com/dizzyd/prospects/world/WorldData.java

Line 149 in 008cb12

bs = world.getBlockState((new BlockPos(x + j, i, z+ k)));

Consider using a MutableBlockPos allocated once before the x,y,z loops, updated with setPos().

Also, I suspect lag spikes during save in

prospects/src/main/java/com/dizzyd/prospects/world/WorldData.java

Line 75 in 008cb12

public NBTTagCompound writeToNBT(NBTTagCompound compound) {

. Can you add a execution length log to it?
Here's our current world for reference, that file seems pretty complex for something saved with the world:
ProspectWorldData.zip

[Derpford]

I've got a similar problem, but probably a different cause: Prospects is creating massive amounts of cascading chunk loads with OreSpawn installed. OreSpawn and Quark both cause a small amount of cascading chunk loads, but as seen in this log excerpt the vast majority of cascaded chunk loads are tagged as belonging to Prospects.