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

improve: save system (performance & safety) #445

Closed
wants to merge 22 commits into from

Conversation

PoetaKodu
Copy link
Contributor

@PoetaKodu PoetaKodu commented Apr 10, 2023

Goal

To make the save system instant and with a safer API.

Progress

Improve performance:

  • single-pass write
    Current: records/entries always ensure that parent folders exist using a search that becomes sluggish when dealing with tens of thousands of files and directories
    Proposed: rewrite the code so that the directory structure is guaranteed and no additional checks are required
  • faster text ops during save entry composition
    Current: using std::stringstream to manipulate text with setEntry(...)
    Proposed: descent one folder/file at a time and use std::to_string() only when needed. Also: test whether we can use std::to_chars()
  • compress after saving
    Current: each file larger thank 256B is compressed during the writing process
    Proposed: create an uncompressed archive and defer compression to another thread after the game is saved and running (configurable)
  • add a configuration to limit the preview image size
    Current: image captures everything in the game window
    Proposed: default to max width of 800px (and automatic height) and expose configuration field to set a custom size
  • lazily load saves when browsing the save list

Improve security

  • merge reading and writing
    Current: reading and writing use two separate pipelines meaning it is very easy for them to get out of sync in the future
    Proposed: create a per-class scheme that handles most of saving details, allowing for logic override if needed

Fix bugs:

  • typo in priview.png change to preview.png

@PoetaKodu PoetaKodu changed the title improve: save system (performance) improve: save system (performance & safety) Apr 11, 2023
@Try
Copy link
Owner

Try commented Apr 11, 2023

using std::stringstream to manipulate text

FYI: there is string_frm class around, that can be used to replace stringstream. std::format_* is not used, because gcc had(and still) no support for it.

@PoetaKodu
Copy link
Contributor Author

PoetaKodu commented Apr 11, 2023

FYI: there is string_frm class around, that can be used to replace stringstream

Ok, good to know. I'll leave it for later though because it doesn't have the key performance impact.

std::format_* is not used, because gcc had(and still) no support for it.

Sure, how about we use fmtlib for the time being?

@Try
Copy link
Owner

Try commented Apr 12, 2023

Sure, how about we use fmtlib for the time being?

string_frm already there and work :)
also we probably still wont be able to use fmtlib/std::format straight, as small-ish strings are common, and used in many places, including animation.
string_frm has explicit control over per-allocated stack space + dynamic allocation, if stack space is not enough.

@PoetaKodu
Copy link
Contributor Author

@Try
btw. I see that string_frm uses std::snprintf under the hood which is much slower than fmtlib's implementation. The library provides fmt::format_to_n() which can be used on preallocated buffer so we could make use of it anyway:

string_frm str;
// assuming we implement .data() and .bufLength()
fmt::format_to_n(str.data(), str.bufLength(), "my text {:.2f}", andNumber);

It can also be used under the hood of the string_frm just to get additional performance.

@Try
Copy link
Owner

Try commented Apr 16, 2023

fmt::format_to_n

Initial intent was to use std::format_to_n, but due to compilers we not there yet. No need to rush it, can simply switch to std, around next year (or when gcc is ready).

@PoetaKodu
Copy link
Contributor Author

Sure

@PoetaKodu PoetaKodu closed this Apr 18, 2023
@PoetaKodu PoetaKodu deleted the improve-saves branch April 18, 2023 20:30
@Nindaleth
Copy link
Contributor

Hey @PoetaKodu, I've noticed you closed the PR rather suddenly. Is this something you'd like to get back to later?

@PoetaKodu
Copy link
Contributor Author

@Nindaleth I'm preparing a better structured version of this PR.

@thokkat thokkat mentioned this pull request May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants