Skip to content

Avoid redundant ZIP metadata decoding when loading saves - #988

Merged
Try merged 2 commits into
Try:masterfrom
Solessfir:improve-save-archive-reading
Sep 14, 2026
Merged

Try merged 2 commits into
Try:masterfrom
Solessfir:improve-save-archive-reading

Conversation

@Solessfir

Copy link
Copy Markdown
Contributor

Avoid repeated ZIP metadata decoding when loading saves and release reader allocations when a Serialize object is destroyed.

  • Count direct children using entry names instead of full file stats.
  • Extract entries by their already-resolved index, reuse the output buffer, and report extraction/CRC errors.
  • End ZIP readers on destruction and disable unsafe moves, since callbacks retain the object's address.

No save-format changes, Android code, Ikarus changes, world caching or background saving.

On a roughly 35,000-entry save, the instrumented directory-counting stage dropped from about 1.3 seconds to 3 milliseconds. This is a stage measurement, not an overall loading-time claim.

Standalone archive tests passed with MSVC Release, covering direct-child counting, long names, stored/compressed entries, empty entries and CRC failures:

cmake -S tests/savearchive -B build/savearchive
cmake --build build/savearchive --config Release
ctest --test-dir build/savearchive -C Release --output-on-failure

@Solessfir

Solessfir commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

Separately, I have two larger save/load experiments in the fork: a bounded cache of immutable current-level data (fresh NPC/script state on every load), and background ZIP compression after capturing an owned save snapshot. Neither is included here. Would either design be worth discussing for upstream before I prepare a separate PR?

@Solessfir

Copy link
Copy Markdown
Contributor Author

For context, measured on the same Archolos save:

  • Current-level cache: repeated reloads took 8,610-9,310 ms with the cache disabled and 4,080-4,730 ms with it enabled under similar heated conditions. Initial cooler reloads were 2,170-2,450 ms.
  • Background save compression: the gameplay-blocking save pause dropped from 10,390-11,430 ms to about 350 ms. ZIP compression and writing continue after gameplay resumes.

These are device measurements rather than controlled benchmarks, but the differences are large and repeatable.

@Solessfir

Copy link
Copy Markdown
Contributor Author

Also feel free to close it this PR, if not needed :)

@Try

Try commented Sep 13, 2026

Copy link
Copy Markdown
Owner

Hi @Solessfir !

Is it pretty-much replacing of mz_zip_reader_file_stat -> mz_zip_reader_get_filename and rest is just moving code around?

@Solessfir

Copy link
Copy Markdown
Contributor Author

Mostly, but there are two separate hot paths:

  • Directory counting uses mz_zip_reader_get_filename instead of decoding full file stats.
  • Entry extraction uses mz_zip_reader_extract_to_callback instead of file_stat followed by extract_file_to_mem, so metadata is not decoded once for the size and again during extraction. It also reuses the existing vector buffer.

The remaining changes include calling mz_zip_reader_end for read archives and deleting the move operation because miniz callbacks retain the Serialize object's address. The helpers and tests separate and cover those paths.

So the main idea is avoiding full file stats where they are not needed, but the rest is not only moving code around.

@Try

Try commented Sep 13, 2026

Copy link
Copy Markdown
Owner

followed by extract_file_to_mem, so metadata is not decoded once for the size and again during extraction

So the whole performance side of idea is to avoid mz_zip_reader_file_stat? This call should be plain O(1), unless you dealing with zip64, what is generally unexpected.

The remaining changes include calling mz_zip_reader_end for read archives and deleting the move operation because miniz callbacks retain the Serialize object's address.

This one is nice.

The helpers and tests separate and cover those paths.

No need here - this only bloats code-base.

@Solessfir

Solessfir commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Yes, the performance change is avoiding mz_zip_reader_file_stat.

It is O(1) - the expensive part here is its constant work: file_stat converts the DOS timestamp with mktime, copies filename and comment buffers, and derives flags for every entry. directorySize scans roughly 35,000 entries twice while loading the world, even though it only needs each filename. Using mz_zip_reader_get_filename avoids around 70,000 unnecessary mktime calls, which explains the measured difference.

About the helpers and standalone tests - I’ll remove them.

@Solessfir
Solessfir force-pushed the improve-save-archive-reading branch from 8742e4c to f9e6a16 Compare September 14, 2026 07:31
@Solessfir
Solessfir force-pushed the improve-save-archive-reading branch from f9e6a16 to 9984b59 Compare September 14, 2026 07:34
@Try
Try merged commit 794d797 into Try:master Sep 14, 2026
7 checks passed
@Try

Try commented Sep 14, 2026

Copy link
Copy Markdown
Owner

Merged, thanks!

@Solessfir

Copy link
Copy Markdown
Contributor Author

Separately, I have two larger save/load experiments in the fork: a bounded cache of immutable current-level data (fresh NPC/script state on every load), and background ZIP compression after capturing an owned save snapshot. Neither is included here. Would either design be worth discussing for upstream before I prepare a separate PR?

Wanna take a look at these? Can do PRs too. But they change way more but also cut way more time.

@Try

Try commented Sep 14, 2026

Copy link
Copy Markdown
Owner

Wanna take a look at these?

Look for sure, but might take time before merge.
Also, you started with android port, about a week ago - would be nice to nail it first :)

@Solessfir

Copy link
Copy Markdown
Contributor Author

Android port is done. I'm pretty happy with the current state.
These PRs are general improvements from that port. So everyone can benifit from it :)

@Solessfir

Copy link
Copy Markdown
Contributor Author

I don't think that you ever merge my fork, cuz it is 250+ commits and some fetures opinioneted, ex. Witcher style foliage fade near the camera - always hated leaves in the face in it the original game :)

@Solessfir
Solessfir deleted the improve-save-archive-reading branch September 14, 2026 20:51
@Try

Try commented Sep 14, 2026

Copy link
Copy Markdown
Owner

Just break it into parts. I presume apk/activity setup, touch ui, foliage - all can be separated into different PR's

@Solessfir

Copy link
Copy Markdown
Contributor Author

I can spam as many PR as you want. But would be cool to have some guidence of what you wanna see first and do you see full android merge in the end or you want only generic android stuff?

Basically more PR/Project direction you gonna give - the better PRs I'm gonna do :)

@Try

Try commented Sep 14, 2026

Copy link
Copy Markdown
Owner

In term of direction.
I think, it's good idea to start with parity with iOS - to have compilation workflow + basic window/activity, swapchain working.

@Solessfir

Copy link
Copy Markdown
Contributor Author

Got you!

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.

2 participants