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

Saving changes #3684

Merged
merged 10 commits into from
Apr 20, 2016
Merged

Saving changes #3684

merged 10 commits into from
Apr 20, 2016

Conversation

fluffyfreak
Copy link
Contributor

@fluffyfreak fluffyfreak commented Apr 17, 2016

Saving times and sizes:

As pointed out in #3680 our save game files are huge, often on the order of 30MiB.
They also take a noticeable amount of time to save/load even on a good machine.
This PR attempts to address both issues using several methods.

A note on Hex Floats:

With the latest VS2015.2 (that's update 2) I was able to get the Hex floats using %a or %la to work, however they produce noticeably larger files. Instead I changed the FloatToStr and DoubleToStr functions to use a union with the appropriate unsigned integer type to avoid all those memcpy calls and stuck that minor modification of the existing conversion.

JsonUtils:

We previously (de)serialised Vectors and Matrices by writing out each component individually and giving it a separate slot in a Json Array.
This was horribly inefficient, instead I now just (de)serialise the whole thing in a single call to sprintf/sscanf and read/write the string directly to Json. This avoids between 100,000 and 110,000 calls to DoubleToStr/FloatToStr cumulatively.

BinStrToJson/JsonToBinStr:

@ShadowNinja identified this as a performance hog and he was spot on, profiling shows that it took approximately 30% to 40% of our saving time in some circumstances. There's a number of reason for this but mostly it was a mixture of inefficiances and not seeing how the Json Array would store it.
I did several things:

  • preallocate the Json Array.
  • compress the BinStr so there's less to store in the first place.
  • pack the array into uint32_t so there's even fewer Json Array entries. Skip this.

All of that brings the typical array size down from ~775,000 to 32,000 ~128,000 each with a uniform sized integer.
It now takes 5% to 7% of the save time.

Odds and sods:

Like @ShadowNinja suggested I switched to the Json::FastWriter which reduced the file size dramatically. Finally I compress the whole file on saving and decompress on loading to reduce the final amount written to disk.

Results:

I'll update with timings but it certainly feels a lot faster saving, i'd guessestimate between 3 and 5 times faster, the profiling that I have supports that approximate measure but I'll need to do more thorough sampling. The results are difficult to replicate as the game state is always so different between runs that you can have 20,000 function differences in each run.

File size is equally problematic however for Earth with the 2016-4-4 build I get save file size of 15,337KiB, and with this PR I get 459 KiB or 1/33rd of the size.

This is obviously also going to be a lot faster when loaded/saved on a phyiscal HDD rather than my fast SSD so others may notice greater performance improvements.

Save Game Bump:

This many changes breaks any chance of compatibility with older save game files.

@ShadowNinja - I hope that I'm not treading on your toes with this but you peaked my interested and I haven't seen you reply for about 6 days, we get a lot of people passing through and this was an easy win to implement.
Thankyou for pointing out how bloated and slow all of this code was and coming up with concrete suggestions for how to deal with it.

Andy

@fluffyfreak
Copy link
Contributor Author

There's some issues with loading on Linux, might need some work.

@walterar
Copy link
Contributor

walterar commented Apr 17, 2016

Curiously, I could compile this PR smoothly in Linux64 and MXE. Problems with Travis?
The size and time load differences are enormous. The savegame docked on Olympus Mons with 3684, 518.8 kB. Without 3684, 19.5 MB

@fluffyfreak
Copy link
Contributor Author

@walterar it seems the it's the loading I had trouble with, are you saying that you can load and save in Linux?

@walterar
Copy link
Contributor

@fluffyfreak In the first load attempt, it frozen. Then he load OK. Problems with previous cache?
Everything seems to work fine now, but best a test longer.

@walterar
Copy link
Contributor

In version W32 (compilation MXE tested in wine) no previous cache problem. All work well, but I guess a work of this magnitude needs more testing time.

@walterar
Copy link
Contributor

walterar commented Apr 18, 2016

@fluffyfreak After many hours of actual game I stumbled upon the first freeze in both, Windows and Linux. I could not catch the mistake of Linux, but catch in Wine.

"wine: Unhandled page fault on read access to at address 0x00000000 (nil) (thread 0009), starting debugger..."

To reproduce (although not always): enter an uninhabited system, advance the time, four or five days, and save the game. While trying to load, the error occurs. :(

@fluffyfreak
Copy link
Contributor Author

Thanks @walterar that's helpful as I no-longer get the crash on my linux machine!
It should help me track it down.

@fluffyfreak
Copy link
Contributor Author

@walterar ok it seems much more stable now, there's more error checking.
The cause of that 0x00000000 was that it had failed to decompress the unpacked byte stream.
I think I must have gotten something wrong in there so I've removed the packing/unpacking code for now.

It's bumped the save game file size up to ~550KB from ~465KB which I can still live with :)

@fluffyfreak fluffyfreak removed the WIP label Apr 19, 2016
@fluffyfreak
Copy link
Contributor Author

Tested on Windows, Linux and OSX.
All worked.

@walterar
Copy link
Contributor

@fluffyfreak Simply brilliant!
The errors report is a configuration problem of Travis, maybe.
I will continue with the tests, but very happy now.

@fluffyfreak fluffyfreak merged commit 6ba3820 into pioneerspacesim:master Apr 20, 2016
@fluffyfreak fluffyfreak deleted the saving-changes branch April 20, 2016 08:14
@ShadowNinja
Copy link

ShadowNinja commented Apr 22, 2016

Eh, I guess my patch will conflict with this pretty bad. Sorry for not getting back to you for a while, I was working on some other things.

Some other things you should do:

  • Don't use BinStrToJson for the "lua_modules" array. Serializing it as a regular string works fine and takes about 1/8th of the space. This was the single biggest change in terms of file size difference in my patch. (EDIT: You say it doesn't work with unprintable characters? If so, that's a JSONCPP bug)
  • Don't Store an array of objects containing only a "frame"/"sfx"/etc field, just store an array of frames.
  • Change "f%f" to "f%g" in the Lua serializer (removes extra zeros at the end).
  • Use incremental table id's, instead of casting their pointer to double.
  • Remove "signature" and "trailing_signature".

Here's what I have so far (doesn't actually work yet): http://ix.io/xmY/diff

@fluffyfreak
Copy link
Contributor Author

fluffyfreak commented Apr 23, 2016

The BinStrToJson problem is that what gets serialised from Lua can escape the JSON string and thus corrupt the JSON format. It's just text and the Lua serialiser can contain ", quite legally.

I'll see what I can pick out or reimplement from your diff, thanks :)

@impaktor
Copy link
Member

Oh, we bumped the save game? Missed that. I've been thinking about reducing the radius of the explored universe (currently 700 ly), which would need a save bump, I think.

@ShadowNinja
Copy link

@fluffyfreak: Doesn't JSONCPP properly escape ", as \",?
If not, there are several other places in the file where LuaSerializer data is saved as a string that would have to be fixed.

@laarmen
Copy link
Contributor

laarmen commented Apr 27, 2016

Ideally, the whole LuaSerializer would be ported to native JSON instead of
our custom "sort of plain text" format.

On Wed, Apr 27, 2016 at 5:37 AM ShadowNinja notifications@github.com
wrote:

@fluffyfreak https://github.com/fluffyfreak: Doesn't JSONCPP properly
escape ", as ",?
If not, there are several other places in the file where LuaSerializer
data is saved as a string that would have to be fixed.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#3684 (comment)

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

Successfully merging this pull request may close these issues.

5 participants