Poweruser
Assigned
Reported
Comments
Michiel, are you aware that Spigot does not save the structure files, unless you specifically tell it to do so? And even if you enable it, it just skips the mineshafts, no matter what you set.
EDIT: Actually, I just went over their code changes again: They are saving the files, just not loading previous saved ones. Either way ...
This is definitely not helpful for Mojang.
EDIT2: The first part of your diskIO log is helpful though, as it adds another piece to the puzzle.
Anthony and I have instead found a way which drastically lowers the time spent for saving compressed NBT files altogether, not just structures. This bug has been in previous versions of Minecraft already, it just hasn't been noticed before.
Added a more complete profiling.
My vote goes for the BufferedOutputStream now. Changing its buffer size during the tests didn't really make a difference.
I did some profiling of the class NBTCompressedStreamTools.
(check the diagram in the attachments area)
With and without this fix:
https://github.com/Poweruser/MinetickMod-1.7.2/commit/b09ed358542129600d02b5be5c90da47cfe58816
You're right, passing the data along via .toByteArray() is not a good choice here. The class ByteArrayOutputStream also got the method .writeTo(OutputStream out), how about that?
Buffering the data will cut down the number of compressions by a great deal, too, but not as much as doing it in one step.
I found the cause of this issue, and the solution for it is pretty simple.
First of all, what exactly are these .dat structure files:
These files are a gzip compressed NBTTagCompound, which itself is build out of many other NBTTagCompounds. In case of Mineshaft.dat, these can easily be several thousands.
What is wrong with them:
The procedure of saving these files is currently very inefficient.
How are these files saved:
Via OutputStreams, they are layered in this fashion:
DataOutputStream(GZipOutputStream(FileOutputStream(File)))
Saving is initiated at the root NBTTagCompound and from there each child-NBTTag is recursively visited. Each time a NBTTag is visited, it is written to the top output stream (the DataOutputStream object).
Here is the culprit: For each write operation on the DataOutputStream, the GZipOutputStream is instantly compressing the new data. The problem is, that we dont have a single write operation with lots of data, but thousands of write operations with very litte data. The overhead of the GZipOutputStream for each compression is building up here.
The solution:
Instead of writing each NBTTag directly to a compressed stream, gather the data in an uncompressed one first, and
then write it all at once to the compressed stream.
Gathering can be done with a ByteArrayOutputStream:
DataOutputStream(ByteArrayOutputStream())
When done, get the data with a call to .toByteArray() on the ByteArrayOutputStream object and write the byte array to the previous compressed stream.
When done, write the data from the ByteArrayOutputStream to the previous compressed stream via .writeTo(OutputStream out)
==========================================
You can even take it one step further:
1) Create a copy of the data with .clone() on the root NBTTag, which takes only a fraction of what compressing and writing takes
2) Let a seperate thread write the copy to the compressed stream
The NBT format definitely got several issues. A few days ago i found another one, which I document here:
https://github.com/Poweruser/MinetickMod-1.7.2/wiki/Heap-dump-analysis:-Redundant-String-objects
Two more memory related issues i can think of right now:
Each NBTTagCompound node object is initialized with a HashMap, to provide a comfortable mapping of string names to its sub nodes. The problem here is that these HashMaps have a default capacity of 16 fields, whereas the compound node that holds the information about a single mineshaft, has only 5 subnodes (ChunkX, ChunkY, id, a list of children (structure pieces), bounding box). And the different structure pieces have between 4 and 8 nodes. So, the HashMap could be built with a initial capacity of 8 instead of 16 and a load factor of 1 (or somewhere close, for them not to be resized to 16 again). But this fix is not future proof. Once there are compounds with 9 nodes, we're back to 16 again, as in the current HashMap implementation the capacity must be a power of 2 (2, 4, 8, 16, 32, ...).
The amount of HashMaps that accumulates is insane as well. Example with a 1,17MB Mineshaft.dat file (compressed):
This file has 708 recorded mineshafts. Each mineshaft has somewhere between 50 and 220 structure pieces (just a quick check in the NBTExplorer. Lets take 100 here, which is a optimistic average. Most are above 100, only a few are below. This makes a estimate of 70800 HashMaps. Additionally to that you have the HashMap-Entry objects which are about 6 times the number of HashMaps: ~425000 (nodes of a structure piece: 4 - 8)
Edit: The NBT format works kinda well for loading/saving files and after that immediately discarding it (like its done with region files and chunks [yes, they are nbt files as well]), while praying that the object is still in the young generation space of the heap, so the frequent garbage collection there can pick it up. If the object made it to the old generation space in the mean time you're screwed.
But it's not suitable for storing something permenantly in memory.