Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interpret EventCommands in Player at runtime #255

Open
fmatthew5876 opened this issue Oct 5, 2018 · 7 comments
Open

Interpret EventCommands in Player at runtime #255

fmatthew5876 opened this issue Oct 5, 2018 · 7 comments

Comments

@fmatthew5876
Copy link
Contributor

fmatthew5876 commented Oct 5, 2018

One way we could save on memory usage is by switching EventCommand::parameters back to uint8_t at runtime. This would mean we have to interpret multi-byte integers on the fly in the interpreter.

Right now we use uint32_t, which has a 4x size overhead of uint8_t. The number of multi-byte integers is very small so we are likely to gain that full 4x savings of parameter memory by conversion.

This would be a big change. Player and Editor would need to be updated to interpet multi-byte integers on the fly. This is just some shifts and logicals operations, which are very cheap. The reduced cache pressure from the memory savings might even make interpreting run faster, despite the additional logical ops.

I did a quick test of just changing the type to uint8_t and loading the database:

Game Memory uint32_t Memory uint8_t Saving
Heros Realm 300 MB 260 MB Saving 13%
HH3 136.4 MB 121.1 MB Saving 11%
@Ghabry
Copy link
Member

Ghabry commented Oct 26, 2018

I would prefer some kind of lazy loading for the worst chunks. I mean for Heros Realm at least the Battle events chunks are >90% of the whole memory usage and except for really long play times you usually won't encounter the hughest part of the enemies, which wastes lots of memory.

A different extreme is Violated Heroine which has >1000 common events and all the RAM is eaten there. Here lazy loading makes no sense because all common events with autorun or parallel must be always loaded and common events are on a hot codepath, checking if they are loaded (which will be almost all) wastes a branch here the uint8_t feature could help a lot!

@fmatthew5876
Copy link
Contributor Author

fmatthew5876 commented Oct 26, 2018

I would prefer some kind of lazy loading for the worst chunks. I mean for Heros Realm at least the Battle events chunks are >90% of the whole memory usage and except for really long play times you usually won't encounter the hughest part of the enemies, which wastes lots of memory.

For battles some kind of LRU cache could make a lot of sense. A background thread can look at random encounters and events (important for those of us who use custom encounter systems!) on the current map to know which battles are possible and pre-cache those.

Even for a long play, you won't be fighting all battles all the time. So some kind of cache where old battles not recently fought drop out of memory should work well.

@fmatthew5876
Copy link
Contributor Author

fmatthew5876 commented Oct 31, 2018

Given the results from #253,

I believe we should just copy all the event stream from LDB/LMU directly into memory as is (except maybe encoding conversions) and decompress/interpret it on the fly in player Interpreter. Everything, including event commands.

Parsing the event stream should not be slow, and we stand to gain significant memory savings. By doing this we would cut our runtime memory usage in half at least, and possibly more.

For example,

From #253 compressing the string reduced heroes realm from 300MB to 200MB. Above in this thread, we saw that using uint8_t parameter type shaved off another 40MB. So we could expect Heros Realm ldb after this issue is done to weigh in no more than 160MB, and quite possibly even less.

@fmatthew5876 fmatthew5876 changed the title Should EventCommand parameters be uint8_t? Interpret EventCommands in Player at runtime Oct 31, 2018
@fmatthew5876
Copy link
Contributor Author

fmatthew5876 commented Aug 18, 2020

Lets try keeping the whole event stream as unserialized

So with this patch:

diff --git a/generator/csv/fields.csv b/generator/csv/fields.csv
index 33137dd..a789529 100644
--- a/generator/csv/fields.csv
+++ b/generator/csv/fields.csv
@@ -127,8 +127,8 @@ CommonEvent,name,f,String,0x01,'',0,0,String
 CommonEvent,trigger,f,Enum<CommonEvent_Trigger>,0x0B,0,0,0,Integer
 CommonEvent,switch_flag,f,Boolean,0x0C,False,0,0,Flag
 CommonEvent,switch_id,f,Ref<Switch>,0x0D,1,0,0,Integer
-CommonEvent,event_commands,t,EventCommand,0x15,,1,0,Integer
-CommonEvent,event_commands,f,Vector<EventCommand>,0x16,,1,0,Array - rpg::EventCommand
+CommonEvent,event_commands,t,UInt8,0x15,,1,0,Integer
+CommonEvent,event_commands,f,Vector<UInt8>,0x16,,1,0,Array - rpg::EventCommand
 Skill,name,f,String,0x01,'',0,0,String
 Skill,description,f,String,0x02,'',0,0,String
 Skill,using_message1,f,String,0x03,'',0,0,String - RPG2000
@@ -294,8 +294,8 @@ TroopPageCondition,turn_actor_b,f,Int32,0x15,0,0,1,Integer - RPG2003
 TroopPageCondition,command_actor_id,f,Ref<Actor>,0x16,1,0,1,Integer - RPG2003
 TroopPageCondition,command_id,f,Ref<BattleCommand>,0x17,1,0,1,Integer - RPG2003
 TroopPage,condition,f,TroopPageCondition,0x02,,1,0,rpg::TroopPageCondition
-TroopPage,event_commands,t,EventCommand,0x0B,,1,0,Integer
-TroopPage,event_commands,f,Vector<EventCommand>,0x0C,,1,0,Array - rpg::EventCommand
+TroopPage,event_commands,t,UInt8,0x0B,,1,0,Integer
+TroopPage,event_commands,f,Vector<UInt8>,0x0C,,1,0,Array - rpg::EventCommand
 Troop,name,f,String,0x01,'',0,0,String
 Troop,members,f,Array<TroopMember>,0x02,,1,0,Array - rpg::TroopMember
 Troop,auto_alignment,f,Boolean,0x03,False,0,1,Flag
@@ -655,8 +655,8 @@ EventPage,overlap_forbidden,f,Boolean,0x23,False,1,0,Flag
 EventPage,animation_type,f,Enum<EventPage_AnimType>,0x24,0,1,0,Integer
 EventPage,move_speed,f,Enum<EventPage_MoveSpeed>,0x25,3,0,0,Integer
 EventPage,move_route,f,MoveRoute,0x29,,1,0,rpg::MoveRoute
-EventPage,event_commands,t,EventCommand,0x33,,1,0,Integer
-EventPage,event_commands,f,Vector<EventCommand>,0x34,,1,0,Array - rpg::EventCommand
+EventPage,event_commands,t,UInt8,0x33,,1,0,Integer
+EventPage,event_commands,f,Vector<UInt8>,0x34,,1,0,Array - rpg::EventCommand
 Event,name,f,String,0x01,'',0,0,String
 Event,x,f,Int32,0x02,0,0,0,Integer
 Event,y,f,Int32,0x03,0,0,0,Integer
@@ -908,8 +908,8 @@ SaveTarget,map_x,f,Int32,0x02,0,0,0,int
 SaveTarget,map_y,f,Int32,0x03,0,0,0,int
 SaveTarget,switch_on,f,Boolean,0x04,False,0,0,bool
 SaveTarget,switch_id,f,Ref<Switch>,0x05,0,0,0,int
-SaveEventExecFrame,commands,t,EventCommand,0x01,0,1,0,int
-SaveEventExecFrame,commands,f,Vector<EventCommand>,0x02,,1,0,event command list
+SaveEventExecFrame,commands,t,UInt8,0x01,0,1,0,int
+SaveEventExecFrame,commands,f,Vector<UInt8>,0x02,,1,0,event command list
 SaveEventExecFrame,current_command,f,Int32,0x0B,0,0,0,int
 SaveEventExecFrame,event_id,f,Int32,0x0C,0,0,0,0 if it's common event or in other map
 SaveEventExecFrame,triggered_by_decision_key,f,Boolean,0x0D,False,0,0,Event was triggered by the Action Key

Max memory usage

Game LDB Size Master w/ patch String PR #397 w/ patch
HH3 16MB 23.6MB 23.3MB
HeroesRealm 27MB 43.6MB 43.2MB
Violated Heroine 11MB 19.0MB 18.4MB
Yume 2kki 1.4MB 3.7MB 3.3MB

The conclusion is obvious, we really cannot parse all events on startup. Especially battle events. Doing delayed parsing would significantly improve our memory usage on games.

It's not clear if we should parse the entire event before it executes, or parse the events 1 by 1 as the interpreter executes. The latter seems more optimal, as we'll defer the work across time. We could also come up with a really optimal parser in speed and memory usage to mitigate the added cpu cost at runtime.

I can't imagine unpacking a few ints each frame will cost much overall, but it could be significant for large chunks of variable math commands all executing on the same frame.

We should also do all re-encoding in liblcf at start time. Basically decode the byte stream, pull out any strings, re-encode them, and the write the byte stream back out to memory.

Everything said above applies to EventCommand and MoveCommand.

@Ghabry
Copy link
Member

Ghabry commented Aug 18, 2020

Okay I'm convinced that this is the only way to go because the memory usage is so extreme low it will help ALL our ports that have oom issues.
Even for common events this would reduce the memory usage: Initially only the events that are unconditional autorun and parallel will be parsed.

But there should be helper functions in event command for doing this

And the XML dump should still emit the real event commands. I use this feature.

@fmatthew5876
Copy link
Contributor Author

And the XML dump should still emit the real event commands. I use this feature.

Should we repurpose this xml format as an output / debugging tool?

@Ghabry
Copy link
Member

Ghabry commented Aug 18, 2020

When the XML format just contains the entire unprocessed event stream it is not really useful because it will look like a binary dump.

I use this file alot to search in projects when analysing bug reports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants