Minecraftforge: ItemLayerModel should pack the quads when baking

[williewillus]

tldr: all builtin/generated models retain their unpacked vertex data in memory which forms a big overhead (duplicated unused vertex data!) when lots of items are present

From some poking around (due to this reddit thread), it seems like item models being kept in unpacked form are taking up quite a bit of memory.

All builtin/generated models are generated using ItemLayerModel, which uses UnpackedBakedQuads. These unpacked quads pack their data when they need to be rendered, but keep the old data lying around. This is very useful in many cases, but it's a huge amount of duplicated (!) vertex data when you have a lot of item models.

I propose that, just in this case, that the model is immediately packed when it's baked and the unpacked data discarded.

I did some experimentation using only 2 content mods, Forestry and EnderIO (+ dependency EnderCore) on a forge dev env, latest 1.10.x branch.

Procedure:

1. Install the 3 mods; ensure the test mods will not be loaded
2. Launch MC with 1G RAM allocated.
3. Launch a The Void superflat world, open the creative search tab and scroll it top to bottom to force all items to render at least once. Let things settle.
4. Open visualvm and examine the memory taken by int[], float[], float[][]...
5. Take note also of where the memory starts at after a GC cycle ingame or in vvm
6. Change ItemLayerModel.buildQuad() to end like this:

UnpackedBakedQuad unpacked = builder.build();
// Immediately pack (getVertexData) to save memory when lots of items are present
return new BakedQuad(unpacked.getVertexData(), unpacked.getTintIndex(), unpacked.getFace(), unpacked.getSprite(), unpacked.shouldApplyDiffuseLighting(), unpacked.getFormat());

1. Repeat steps 1-5
   

   Results:

• Retaining unpacked data:
  
  
  
  • float[]: ~49MB <----- extra data
  
  
  • float[][]: ~12MB
  
  
  • int[]: ~50M
  
  
  • GC cycle falls back to: ~560MB
  
  
• Packing immediately on bake, delete unpacked data:
  
  
  
  • float[]: ~11.2MB
  
  
  • float[][]: ~2.5MB
  
  
  • int[]: ~50M
  
  
  • GC cycle falls back to: ~520MB
  
  

This is ~35MB of duplicated vertex data. Just for 2 mods. Extrapolating to my personal pack with ~9 content mods, this is ~157MB, extrapolating to AllTheMods 1.10.2 with ~40 content mods (I counted "content mods" skipping over a lot of mods that still add items) that's ~700MB

People are already hurting a bit from the new system's increased memory overhead so if we have opportunities to cut obvious memory wasters I think we should.

This'll obviously increase load/bake time but it's merely moving to load time something that'll happen at runtime either way. Also, if the first useless load/bake is eliminated (#2956) it won't hurt that much at all

---

Update:

Tested on my personal pack: total savings ~70MB for ~9 content mods
Tested on AllTheMods 1.10.2: total savings ~400MB for ~40 content mods

Smaller than my extrapolations but nonetheless a sizeable amount

[OrionDevelopment]

I would be okay with this, however looking at consistency here; we should then only bake once (after post init) or only discard the unpackedbakedquads once (again after post init).

If we do not do that, we get inconsistencies left and right, in other words during init MC's items + the items you register during pre init lost their unpackedbakedqudas however the items you add for mod compat (during init) have not lost theirs, making it impossible to read the lost data if needed.

[williewillus]

Items should never be registered after preinit, any mod that does that is broken and incorrect.

W. R. T. Your original point, it should just be changed where I noted in the original post. Skipping the first bake is a separate unrelated optimization

Additionally, if a mod needs to unpack specific items they can do that later. We should start everything packed and unpack as needed instead of starting unpacked and then packing and wasting all that space holding unused data.

[Actuarius]

@mezz added labels [Performance]

[DarkGuardsman]

@williewillus There are some cases that registering items after pre-init is needed. For example, content that only loads if another mod loads it's optional content. As well ore-dictionary only content such as metal blocks, molten fluids, and specific ore-dictionary content. Which honestly could be solved with a few events that fire when content is registered or mods finish their pre-init.

That being said I approve of only processing models once during load time. As well of making these adjustments as need to improve performance.

[diesieben07]

The other mod needs to expose whether it will load the optional content then, before preInit.
Or you need to make your mod load before theirs.

[LexManos]

When discussed with fry there were compelling reasons not to have this bake most importantly that the implementation doesn't allow for properly determining which models need this information or not. Feel free to start testing out implementations and working with fry to find something that addresses all issues. However magic "it just works" without dealing with all cases is not worth hyping up.

[williewillus]

re: above

The issue being discussed is not about items after preinit or whatever.
The problem in this ticket is that all item quads are left in unpacked form when most people don't need that data. If they do they can manually unpack it again. We should bake to a compact format and those who need unpacked should opt to unpack, instead of baking to unpacked and leaving it sitting around unused.

The "should items be registered after preinit?" issue has nothing to do with this ticket, that has to do with eliminating the redundant bake, which is #2956

[LexManos]

That exact train of thought is something I discussed with fry as I'm not a moron. My original statement still stands. Speak to fry.

[williewillus]

I haven't been able to be on irc for a while, but any further news on this? Seems like a low-hanging/easy memory optimization with large reward, and it should not break any mods. If in the tiny chance the mod was relying on this to generate unpacked quads that mod can just take the packed ones and reunpack them. I am betting that that count of mods is quite low, though.

[LexManos]

Speaking with @RainWarrior, its not a simple fix. The problem as explained to me is the lighting engines speed requires things to be unpacked.
And the fact is we have no way of telling what needs to run through the lighting engine.
So there is will be demand for this unpacked data depending on how the models are used.
The only option, which ive instructed fry to do, is to wrap this around a config value. So that it will essentially have a 'trade CPU time for memory by packing the values and unpacking them on demand' config.

[DarkGuardsman]

Could also add a way for mods to tell forge when things are needed or not?

On Oct 19, 2016 1:49 PM, "LexManos" [email protected] wrote:

Speaking with @RainWarrior https://github.com/RainWarrior, its not a
simple fix. The problem as explained to me is the lighting engines speed
requires things to be unpacked.
And the fact is we have no way of telling what needs to run through the
lighting engine.
So there is will be demand for this unpacked data depending on how the
models are used.
The only option, which ive instructed fry to do, is to wrap this around a
config value. So that it will essentially have a 'trade CPU time for memory
by packing the values and unpacking them on demand' config.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/MinecraftForge/MinecraftForge/issues/3246#issuecomment-254888741,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABaK0oVv89GkcXcX888MfiScP8Gp8XKaks5q1lgNgaJpZM4J56PK
.

[LexManos]

No already asked about that, its apparently not the mods that need this data its the lighting engine itself. That needs it for all models that get run through it.

[AEnterprise]

could a possible solution be to first clear the remove the unpacked data and then have a cache for the lighting engine? it could unpack everything it needs the first time and then cache it

[williewillus]

Forgive my ignorance about the rendering system, but I can't see where in the code the light engine/pipeline needs things unpacked.

All I see, in LightUtil.putBakedQuad, is that the quad is unpacked only if the source and destination vertex formats do not match sizes. These ItemLayerModels are all in the same VertexFormat as the tessellator (ITEM), so don't see how it's a problem.

Rendering an item, from my tracing, boils down to wr.addVertexData(quad.getVertexData()); (RenderItem.renderQuads and derivatives)
getVertexData() on an unpacked quad makes it immediately pack all of its data. Where is the unpacked data being used?

and regarding "caching", seeing as things like JEI eventually cause all items to render, that would put us in basically the exact same situation we have today (unpacked and packed data both persisting for a large number of models)

[DarkGuardsman]

@LexManos is there any way we can move the memory usage to the hard disk or graphics card. As that might solve some of these issues. Then again I have no clue what I'm saying :/ but feel like someone has solved this issue before. Maybe some of us should start looking into how other games and engines handle this problem. Then maybe abstract & replace how MC's directly handling the data into a better version.

[williewillus]

neither of those is going to work

• hard disk: your OS already does that with swapping/virtual memory. If you mean not generating model data each startup...maybe?
• graphics card: people don't have the necessary vram to hold 400MB+ unused model data there :P I don't at least.

[DarkGuardsman]

@williewillus Ya I was thinking of not generating them each setup and creating a really low access speed cache. Letting the OS do this with swap space is cool but its runs better when the program controls it. As well even if some people do not have graphics cards it can help those that do. For example, I have 2 cards that each can hold 2GB each.

[williewillus]

disk is several times slower than memory so you're going to have a badddd time in an environment demanding low latency like a game

[DarkGuardsman]

Yes, I know the disk will be slower but if combined with something that could predict usage it could save some RAM. For example off loading data that has not been used in 1 min or longer. Them queueing it up for loading if the game is about to render something in the next second. It would require a lot more code and design. However, I think it could work so long as mods like NEI and JEI are not in heavy use. Since those mods would end up rendering everything anyways.

[LexManos]

No.
What will happen is when @RainWarrior has time.
He will implement a config value to disable the lighting engine which is what uses this value.
Downgrading your lighting experience and trading CPU time to save memory will be a option.
When fry gets time.

[bs2609]

Here's the results of a heap dump taken while running All The Mods, if anyone's interested.

[williewillus]

looking back this has implications that I don't fully understand, and bs's quad reduction optimizations + foamfix provide better savings with lower risk, so closing

[bs2609]

3612 would address this if it eventually gets merged.