[CR] Seamless loading of gzipped world/maps/x.y.z/x.y.z.map files #43986

int-ua · 2020-09-06T07:11:11Z

Superseded

Summary

SUMMARY: Performance "Seamless loading of gzipped world/maps/x.y.z/x.y.z.map files"

Purpose of change

Ability to drastically reduce save folder sizes with (hopefully) only a marginal performance impact.

Describe the solution

Checking first two bytes of files before loading JSON and if they are compressed use gzstream instead of std::ifstream to read them.
Reference: kevingranade/Cataclysm@e751f0b

Testing

#!/bin/bash
WORLD_NAME="test"
./cataclysm-tiles --world "$WORLD_NAME"
cp -r "save/${WORLD_NAME}" "save/${WORLD_NAME}_gz"
find "save/${WORLD_NAME}_gz/maps/" -type f -exec gzip "{}" \; -exec mv "{}.gz" "{}" \;
./cataclysm-tiles --world "${WORLD_NAME}_gz"
du -sh "save/${WORLD_NAME}/maps/"
du -sh "save/${WORLD_NAME}_gz/maps/"

Additional context

#10784 #14307 #19192 #24916 #32530

int-ua · 2020-09-06T21:18:58Z

Not sure I will have time in the coming days to work more on this PR unfortunately. But I definitely want this feature and no one else seems to be available and interested simultaneosly.

This reverts commit 8a2b0c1.

int-ua · 2020-09-13T18:43:48Z

~~libgzstream-dev package is not available for Ubuntu Xenial. Is there anything that can be done about that?~~

int-ua · 2020-09-13T20:33:15Z

~~Is it ok if I add gzstream as a git submodule? It's easier to just go with a folder I guess but would you (maintainers) mind if try submodule?~~

jbytheway · 2020-09-14T13:00:19Z

Of course it is up to @kevingranade but I'd strongly recommend against using submodules. In my experience they always cause more issues than they solve. For example, new people checking out the codebase have extra steps they need to take or things behave strangely.

You already ran into a related issue in needing to alter the actions to checkout the submodules. There are so many little places where similar issues will arise; it's just not worth the trouble.

int-ua · 2020-09-14T16:17:21Z

In my opinion the issue is highly outweighed by the benefits of not forcing everyone to download the history of everything. Although it's quite late at this point I still think that this project could benefit from git submodules and I'm trying to establish a precedent. It's just an additional command or two to run that can save you ghiblibytes of space in the future. I'm genuinely interested, what are these little places?

kevingranade · 2020-09-14T17:28:21Z

If you're trying to set a precedent for more extensive use for git submodules, stop now, that's not happening.
I was interested in seeing the outcome of using submodules for an optional dependency, but as I'm positively allergic to taking dependencies in the first place, I don't anticipate there being many opportunities for using them in the future.

I'm not clear what the mechanism would be for having people not download the entire project history. We can possibly shift out the tilesets, but even that would be a massive undertaking of rebasing the entire project history to remove them from the main repository, and nothing else is coming to mind that is optional for download.

jbytheway · 2020-09-14T17:45:18Z

Since you asked, here are some of the annoyances from submodules I'm aware of:

New users/developers need to do the extra commands to get everything. This is yet another obstacle to the already-large issues with learning git that new contributors have, especially on a project like this where many contributors are not previously git users.
If you clone the repo without the submodule and try to work somewhere without Internet access then it's too late to download the submodule.
GitHub zip downloads of repos don't include submodules.
git archive doesn't include submodules (which is the reason for the above point).
Every system that automatically obtains and builds the code needs to be updated to take account of this. This is not just every CI system we set up for the project, but also each one of the many Linux distros that ship a CDDA package.
If the module you reference ceases to exist or rewrites its history then historical versions of CDDA become impossible to build, because the referenced commits cannot be found. Even if the referenced repo merely moves to a new host, then you must use little-known git configuration options to redirect to the new location, and you have to change that configuration depending on the point in history you're trying to build, which is messy at best.

int-ua · 2020-09-14T17:48:12Z

stop now, that's not happening.

ok, I'll just copy the files.

optional for download

It's becoming off-topic and irrelevant here but just to explain my opinion, in the order of being suitable for offloading into submodules (more to less):

gfx/*: I forgot to include these at first, added by editing.
data/mods/*: as a person who mostly plays default/vanilla
android and maybe other platform-specific folders
lang/po: as a person who plays without l10n. Although I've tried Transifex for my lang but it's just too slow. This folder is larger than everything else combined.

olanti-p

Here's a bunch of changes that should make it compile / be a bit cleaner

.github/workflows/CBA.yml

src/gzstream.cpp

src/cata_utility.cpp

src/gzstream.h

Co-authored-by: Oleg Antipin <60584843+olanti-p@users.noreply.github.com>

int-ua · 2020-09-15T19:47:24Z

Thank you! Let's see if I missed anything...

int-ua · 2020-09-15T20:12:59Z

fatal error: SDL_version.h: No such file or directory

Is it really a problem that was introduced by this PR? Is g++-7 build different in that it needs something like one of the configs in CMakeModules/?

jbytheway · 2020-09-16T01:27:40Z

CMakeLists.txt

@@ -369,6 +369,8 @@ IF(USE_HOME_DIR)
 ENDIF(USE_HOME_DIR)

 add_subdirectory(src)
+add_executable(main src/main.cpp)


You don't want to add a new executable like this; it needs to be made a dependency of the existing executables.

zlib is already a dependency of the binaries in TILES mode, so you can see how this is done (in src/CMakeLists.txt, not this file) and mimic that. Will need something like

target_include_directories(cataclysm-common PUBLIC ${ZLIB_INCLUDE_DIRS}) target_link_libraries(cataclysm-common ${ZLIB_LIBRARIES})

Not realizing there is a src/CMakeLists.txt was my major problem until now, thank you!

int-ua · 2020-09-16T11:20:38Z

Still ld: ../src/CMakeFiles/cataclysm-tiles-common.dir/gzstream.cpp.o: undefined reference to symbol 'gzread'

int-ua · 2020-09-16T17:27:59Z

//lib/x86_64-linux-gnu/libz.so.1: error adding symbols: DSO missing from command line
Ok, looks like g++-7 is still missing -lz somewhere.

int-ua · 2020-09-16T17:53:22Z

/usr/local/android-sdk-25.2.3/ndk-bundle/build/core/build-binary.mk:651: Android NDK: Module main depends on undefined modules: z

jbytheway · 2020-09-17T00:51:05Z

I looked at the src/CMakeLists.txt a little more closely. It appears that zlib is only being made a dependency of cataclysm-tiles-common when DYNAMIC_LINKING is OFF (which is not the default). That dependency needs to be made unconditional; you can move the ZLIB parts down to the target_include_directories and target_link_libraries after ENDIF (NOT DYNAMIC_LINKING).

blindly outline support for seamless loading of gzipped map files

2254c49

int-ua changed the title ~~[CR] Support for seamless loading of gzipped map files~~ [CR] Draft for support for seamless loading of gzipped map files Sep 6, 2020

anothersimulacrum added the [C++] Changes (can be) made in C++. Previously named `Code` label Sep 6, 2020

int-ua added 10 commits September 13, 2020 17:14

almost compiling

e40406a

Added libgzstream-dev to .travis.yml

b72e746

trying to fix 'undefined reference' error

99db84a

astyle

5799f3d

revert HEAD^^

65cbd76

added libgzstream-dev to the Dockerfile

11970d4

dependencies

0e1b625

changed path data type

8a2b0c1

Revert "changed path data type"

cc79bd5

This reverts commit 8a2b0c1.

LDFLAGS

9dd29d7

-lz flag

14421f7

int-ua added 8 commits September 14, 2020 10:46

gzstream submodule

4de0f28

Removing libgzstream-dev dependency

a4d27c9

gzstream license

e74a0ab

trying CXXFLAGS -I

3e7502c

submodule option in the checkout action

4c43b74

using -isystem for gzstream library

9025dae

CMakeLists.txt add_subdirectory

e12c50a

CMakeLists include_directories

4c3265c

int-ua added 3 commits September 15, 2020 21:02

trying to fix more warnings

e7a12e5

removed #define

f4cbec9

"did you mean?" Yes, I meant whatever works

7f4806a

olanti-p reviewed Sep 15, 2020

View reviewed changes

int-ua and others added 2 commits September 15, 2020 22:34

Update .github/workflows/CBA.yml

57d170d

Co-authored-by: Oleg Antipin <60584843+olanti-p@users.noreply.github.com>

getting rid of gzstream namespace

da14ad9

Co-authored-by: Oleg Antipin <60584843+olanti-p@users.noreply.github.com>

int-ua force-pushed the compression branch from 969f8bd to da14ad9 Compare September 15, 2020 19:44

jbytheway reviewed Sep 16, 2020

View reviewed changes

int-ua added 3 commits September 16, 2020 12:13

moved cmake zlib requirement

f532f43

moving zlib around in src/CMakeLists.txt

5829d0b

put FIND_PACKAGE(ZLIB) back

3340b86

android libz

447b597

moved -lz

c5919df

int-ua added 4 commits September 16, 2020 20:54

android: zlib?

786bb09

android: -lz?

6bfc712

android build working. Now g++-7. CXXFLAGS -lz?

cb1be6d

Not -lz in CXXFLAGS

b16d2a2

int-ua changed the title ~~[CR] Draft for support for seamless loading of gzipped map files~~ [CR] Support for seamless loading of gzipped save/map/x.y.z/x.y.z.map files Sep 16, 2020

int-ua changed the title ~~[CR] Support for seamless loading of gzipped save/map/x.y.z/x.y.z.map files~~ [CR] Support for seamless loading of gzipped world/maps/x.y.z/x.y.z.map files Sep 16, 2020

int-ua changed the title ~~[CR] Support for seamless loading of gzipped world/maps/x.y.z/x.y.z.map files~~ [CR] Seamless loading of gzipped world/maps/x.y.z/x.y.z.map files Sep 16, 2020

unconditional zlib

ab412c1

int-ua mentioned this pull request Sep 17, 2020

Performance "Seamless loading of gzipped JSON/txt files (data and map)" #44218

Merged

int-ua closed this Sep 17, 2020

int-ua deleted the compression branch December 21, 2020 13:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CR] Seamless loading of gzipped world/maps/x.y.z/x.y.z.map files #43986

[CR] Seamless loading of gzipped world/maps/x.y.z/x.y.z.map files #43986

int-ua commented Sep 6, 2020 •

edited

Loading

int-ua commented Sep 6, 2020 •

edited

Loading

int-ua commented Sep 13, 2020 •

edited

Loading

int-ua commented Sep 13, 2020 •

edited

Loading

jbytheway commented Sep 14, 2020

int-ua commented Sep 14, 2020

kevingranade commented Sep 14, 2020 •

edited

Loading

jbytheway commented Sep 14, 2020

int-ua commented Sep 14, 2020 •

edited

Loading

olanti-p left a comment •

edited

Loading

int-ua commented Sep 15, 2020

int-ua commented Sep 15, 2020

jbytheway Sep 16, 2020

int-ua Sep 16, 2020

int-ua commented Sep 16, 2020

int-ua commented Sep 16, 2020

int-ua commented Sep 16, 2020

jbytheway commented Sep 17, 2020

[CR] Seamless loading of gzipped world/maps/x.y.z/x.y.z.map files #43986

[CR] Seamless loading of gzipped world/maps/x.y.z/x.y.z.map files #43986

Conversation

int-ua commented Sep 6, 2020 • edited Loading

Superseded

Summary

Purpose of change

Describe the solution

Testing

Additional context

int-ua commented Sep 6, 2020 • edited Loading

int-ua commented Sep 13, 2020 • edited Loading

int-ua commented Sep 13, 2020 • edited Loading

jbytheway commented Sep 14, 2020

int-ua commented Sep 14, 2020

kevingranade commented Sep 14, 2020 • edited Loading

jbytheway commented Sep 14, 2020

int-ua commented Sep 14, 2020 • edited Loading

olanti-p left a comment • edited Loading

Choose a reason for hiding this comment

int-ua commented Sep 15, 2020

int-ua commented Sep 15, 2020

jbytheway Sep 16, 2020

Choose a reason for hiding this comment

int-ua Sep 16, 2020

Choose a reason for hiding this comment

int-ua commented Sep 16, 2020

int-ua commented Sep 16, 2020

int-ua commented Sep 16, 2020

jbytheway commented Sep 17, 2020

int-ua commented Sep 6, 2020 •

edited

Loading

int-ua commented Sep 6, 2020 •

edited

Loading

int-ua commented Sep 13, 2020 •

edited

Loading

int-ua commented Sep 13, 2020 •

edited

Loading

kevingranade commented Sep 14, 2020 •

edited

Loading

int-ua commented Sep 14, 2020 •

edited

Loading

olanti-p left a comment •

edited

Loading