-
Notifications
You must be signed in to change notification settings - Fork 55
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
Optimize memory usage with DBString and DBArray #379
Conversation
Will it still be possible to allocate a new DBString and overwrite an existing DBString? |
Right now DBString just does That's why I'm suggesting either a separate build mode, or some template way of creating a Essentially we need "fast read-only player mode" and "slow read/write editor mode" for liblcf somehow. |
Maybe the editor thing should be a separate physical library + headers somehow deps the base I don't like the idea of BUILD modes. I would prefer if people install a binary liblcf package, they get everything they need for both Player and Editor. As much as I hate adding a crap ton more auto generated headers to liblcf this is probably the way to go. For player we should only compile against and link to the fast implementation. We don't want to pay compile or link time costs for the slow one. For editor we don't care. One might even want the fast implementation available so we can provide expected memory usage statistics in the editor. |
I pushed an experiment with deduplication. Basically each string is refcounted and we look it up in a hash table. Unfortunately the results are not great. We get a meager savings in memory and a slower load time. For the memory, I can assume that even though a lot of strings are repeated, the memory overhead of the hash table dwarfs the savings. For the runtime, I could probably improve things by using a faster hash table.
Overall looks like deduplication of strings is not worth it 👎 |
And now lets try taking out the hash table deduplication, but keeping the reference counting mechanism for copies.
Memory usage is slightly up across the board. This is a also a loser. Not surprising, since we have a lot of strings and an extra 4 bytes for each one still adds up. 👎 Pre C++11 would have had a profile like this, as std::string used to be copy on write reference counted. |
I tried playing with the custom allocator, and it didn't produce much in the way of results. Seems like malloc does a pretty good job these days. So this DBString class just being a very naive string which is allocated each time and doesn't support modification seems to fit the bill. This also means for now we don't need to bifurcate liblcf. While this DBString is not super convenient to use like std::string, you can read and write to it from Editor just fine. |
One more important test. What are the memory usage differences if we take events out of the picture? Max memory usage Toop::event_commands removed
Max memory usage Toop::event_commands and CommonEvent::event_commands removed
Event without events, this string type has memory savings for the rest of the database. The conclusion here is that for real wins, we have to either compress or not keep all these event data into memory at once. As has been suggested in the past, I agree probably some caching mechanism would make sense. For battles this is easy, we have several frames of battle transition where we could load the battle info from disk and keep it in an LRU cache. For common events not so much, since they can trigger at any time and even 1 frame delay could break a game. As was also suggested before, compressing the data would help a lot. Basically keeping the event code in it's binary form and decoding it on the fly as the events execute. |
Very nice tests! From an editor pov that the allocator doesn't work is good news ;). Nice work! |
How are the savings when you keep the RPG::eventcommand as is and only make the parameter vector uint8_t with the raw BER data? Should also safe some memory this way and keep the API mostly the same. Another 4 bytes: Will it break games when the code and indent are int16_t? The values would all fit. |
I did that here in #255 But check out the new post if you replace the entire event stream by
I think this would be fine, but I would use Not sure what the indent limit is in RPG_RT, but 255 indents seems like way too many. A better option could be 24 bytes for the code and 8 bytes for the indent. 255 is the sentinal for Another way to reduce 16 bytes, add a Even better, make some Optimizing the event parsing for speed could have measurable effects on script heavy games like Frozen Triggers and Smash Bros. |
17c5128
to
a57871e
Compare
I've switched StringView to use this implementation: https://github.com/martinmoene/string-view-lite We now distribute Also wrote unit tests. I've decided to use DBString for ldb, lmt, and lmu files for now. We can always change this later if we want to and maybe we will just use DBString for all strings. I'm really not thrilled about this: Basically, enable fmtlib printing support without including more than It's ugly, but it works and probably necessary if we don't want to add the dependency at the lcf level. Player PR is up to date and building against this one. This is ready for review. |
src/lcf/third_party/string_view.hpp
Outdated
// string-view lite, a C++17-like string_view for C++98 and later. | ||
// For more information see https://github.com/martinmoene/string-view-lite | ||
// | ||
// Distributed under the Boost Software License, Version 1.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest mentioning it on README.md as we do with inih.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Terms,file,f,DBString,0x94,'',1,0,String | ||
Terms,exit_game_message,f,DBString,0x97,'',1,0,String | ||
Terms,yes,f,DBString,0x98,'',1,0,String | ||
Terms,no,f,DBString,0x99,'',1,0,String | ||
Music,name,f,String,0x01,"(OFF)",1,0,String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are music name and sound name the only non-save chunks shared with savegames?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe so, and maybe we'll just convert all the save data too at some point
src/lcf/third_party/string_view.hpp
Outdated
|
||
typedef Traits traits_type; | ||
typedef CharT value_type; | ||
using char_type = value_type; // <- LIBLCF HACK: to workaround bug in older versions of fmtlib for Player: https://github.com/fmtlib/fmt/issues/1539 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Highlighting this. It's one line I had to add to this implementation to workaround bugs in old versions of fmtlib.
8d6ae1f
to
fb20b52
Compare
I've added I've reproduced the performance numbers here.
This is as far as this PR will go. |
Some numbers for HH3 memory usage in Player at the title screen in Windows 10 release build.
One of the optimizations I do in this implementation matters a lot That is for empty strings, you still need to create a 1 character buffer to the store the null terminator. In my implementation, I create a single Without this, we do a ton of 5 byte mallocs for lots of empty strings
This also greatly improves performance for empty strings, as they all point to the same memory and thus keeps empty strings hot in cpu cache. |
This gives an easy way to disable reading some chunks, for memory profiling
This PR adds some DBArray and #383 adds more DBArray? |
Depends on: EasyRPG/Player#2280
DBString
sizeof(DBString) == sizeof(char*)
DBarray
Performance Measurements
bench_readldb
max memory usagebench_readldb
average runtimeNotes about Editor
DBString
is optimal for read-only database usage of Player but not necessarily for Editor. For editor, maybe we want to still usestd::string
for all strings. Perhaps anEDITOR
build mode of liblcf?Given that Editor is not being worked on, I'm not super interested in putting too much effort on this side.
Todo
Write a custom memory allocator for DBString and benchmark itDBString
move only, or at least make copies reference same underlying string using allocator?DBString
from database should be read only and exist for the lifetime of the game, maps however come and go. How to reconcile with allocator? Maybe DB and Map arenas?Add a "use std::string" for everything build mode for editor and other interactive tools.