Avoid duplication of mod metadata in memory #12562

TurkeyMcMac · 2022-07-19T22:08:00Z

I'd like mod storage to act more like a proper key-value database. It shouldn't store all the data in memory. With this PR, using the SQLite3 backend, the data is no longer kept in memory (unless SQLite3 chooses to do so.) The metadata interface has been split into IMetadata for the basic interface and SimpleMetadata for in-memory metadata. ModMetadata extends IMetadata. The interface only requires all data to be in memory when comparing two metadata maps and when converting metadata to a table. I could avoid this, but I'd rather not implement custom iterators and all that BS.

With this PR, mod writers can store huge amounts of data in mod storage without worrying about RAM limits, provided they are using the SQLite3 backend.

To do

This PR is Ready for Review.

C++ unit tests
Lua unit tests
minetest.features entry

How to test

There are unit tests in Lua and C++. You can also test with mods that use mod storage and other metadata.

Zughy · 2022-08-14T19:07:36Z

@TurkeyMcMac rebase needed

sfan5

(code review only)

sfan5 · 2022-08-25T14:34:30Z

doc/lua_api.txt

@@ -4868,6 +4868,9 @@ Utilities
 get_sky_as_table = true,
 -- VoxelManip:get_light_data accepts an optional buffer argument (5.7.0)
 get_light_data_buffer = true,
+ -- When using an appropriate backend (not the deprecated "files" backend),
+ -- mod storage data is not necessarily all kept in RAM (5.7.0)
+ mod_storage_on_disk = true,


fuzzy as it is what is the practical application?

It means that, when using a non-deprecated backend, the size of mod storage is not limited by the available RAM. This doesn't include the dummy backend, which explicitly keeps everything in RAM. Perhaps I should change this comment.

The documentation has been changed.

No I understand what it means, but what are mods expected to do with this information?

If they detect this feature, they can basically store as much data as they want in mod storage.

They wouldn't have to insecurely require lsqlite unless they needed a full-on relational database.

The problem I see here is that the feature entry only says "maybe it won't all be in RAM". If a mod relies on this and stores lots of data then -- depending on circumstances it cannot determine -- the engine might still run out of RAM.
For a mod the only safe way is to not store much data regardless of this feature flag. Therefore it's not useful.

The updated documentation is more clear about this.

Maybe this diagram makes it clearer.
The problem I see is that mods cannot know for sure (*) that storing lots of data is safe.
(*) It is certainly possible to open world.mt and read the current backend. Aside from being obviously unportable we shouldn't encourage or support this way (API guarantees).

This is fundamentally the same reason why #12167 was rejected.

It's unfortunate, but mods can specify in their READMEs that a backend should be used other than "files" or "dummy".

src/content/mods.cpp

src/database/database-dummy.cpp

src/metadata.cpp

src/metadata.h

src/script/lua_api/l_nodemeta.cpp

src/script/lua_api/l_storage.cpp

The metadata interface is now more general.

Maybe this will fix the gcc_5 CI build?

This ensures that every instance of SimpleMetadata works with nullptr passed as a place argument to getStringRaw.

Just for good measure.

It is no longer necessary to keep just one object per mod, since data is no longer stored in these objects. Now one can obtain multiple distinct references with core.get_mod_storage, but this will be changed by PR minetest#12572. One can test like so: local a = core.get_mod_storage() local b = core.get_mod_storage() a:set_string("!", "?") assert(b:get("!") == "?")

This bug was found by sfan5.

This is possible since registerModStorage was removed, at least.

TurkeyMcMac · 2022-09-26T11:30:30Z

I rebased to remove registerModStorage and unregisterModStorage from DummyGameDef (and to fix conflicts.)

TurkeyMcMac marked this pull request as ready for review July 20, 2022 01:38

sfan5 added @ Script API Roadmap The change matches an item on the current roadmap Feature ✨ PRs that add or enhance a feature labels Jul 20, 2022

TurkeyMcMac force-pushed the mod-meta-dedup branch from 9345583 to d989e4b Compare July 23, 2022 20:52

TurkeyMcMac mentioned this pull request Jul 23, 2022

Let core.get_mod_storage be called multiple times #12572

Merged

TurkeyMcMac mentioned this pull request Aug 12, 2022

unusable teleport_tubes file after updating to 5.6.0 mt-mods/pipeworks#39

Closed

sfan5 added the Rebase needed The PR needs to be rebased by its author label Aug 14, 2022

TurkeyMcMac force-pushed the mod-meta-dedup branch from d989e4b to 010e96a Compare August 14, 2022 19:56

Zughy removed the Rebase needed The PR needs to be rebased by its author label Aug 14, 2022

sfan5 reviewed Aug 25, 2022

View reviewed changes

TurkeyMcMac force-pushed the mod-meta-dedup branch from 010e96a to f25cc0d Compare August 25, 2022 15:47

TurkeyMcMac mentioned this pull request Aug 27, 2022

Consolidate API object code #12728

Merged

sfan5 self-requested a review August 29, 2022 20:05

TurkeyMcMac force-pushed the mod-meta-dedup branch from ddd58e5 to 91a25bd Compare September 13, 2022 18:19

sfan5 approved these changes Sep 13, 2022

View reviewed changes

sfan5 added the One approval ✅ ◻️ label Sep 13, 2022

TurkeyMcMac force-pushed the mod-meta-dedup branch from 91a25bd to c4b798f Compare September 19, 2022 14:20

TurkeyMcMac mentioned this pull request Sep 21, 2022

Store teleport tube DB more compactly mt-mods/pipeworks#45

Merged

TurkeyMcMac added 10 commits September 26, 2022 07:27

Avoid duplication of mod metadata in memory

f19b4a7

The metadata interface is now more general.

Replace TRUE with 1

7315cd3

Maybe this will fix the gcc_5 CI build?

Add metadata unit tests in Lua

3f1e129

Fix use after destruction

8e065ac

Test metadata resolution

5236fab

Add minetest.features flag for mod storage

3233b04

Improve mod meta unit tests

0b614e0

Add some metadata API documentation

0f2bb43

Correct ModMetadata::getStrings according to API

17435b5

Simplify ModMetadata::getStrings

c6bb7c1

TurkeyMcMac and others added 11 commits September 26, 2022 07:27

Make SimpleMetadata::getStringRaw final

fcbf379

This ensures that every instance of SimpleMetadata works with nullptr passed as a place argument to getStringRaw.

Make SimpleMetadata::getStrings final

14c43b2

Just for good measure.

Fix SimpleMetadata removal modification tracking

3e7c46f

Test storage reference equality

feeb608

Make some trivial changes suggested by sfan5

0230d79

Add assert statements suggested by sfan5

47c560f

Test dummy mod metadata database

9551021

Fix Database_Dummy::getModEntry

daeefe8

This bug was found by sfan5.

Clear up description of mod_storage_on_disk

6c14377

Store ModMetadata directly in StorageRef

1abc3bd

This is possible since registerModStorage was removed, at least.

TurkeyMcMac force-pushed the mod-meta-dedup branch from c4b798f to 1abc3bd Compare September 26, 2022 11:29

TurkeyMcMac added 2 commits September 26, 2022 16:00

Fix storage equality test

30404fc

Move compare_meta out of test body

fbe89ee

TurkeyMcMac merged commit f4a01f3 into minetest:master Sep 26, 2022

TurkeyMcMac mentioned this pull request Oct 8, 2022

Add MetaDataRef:get_keys #12841

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid duplication of mod metadata in memory #12562

Avoid duplication of mod metadata in memory #12562

TurkeyMcMac commented Jul 19, 2022 •

edited

Loading

Zughy commented Aug 14, 2022

sfan5 left a comment

sfan5 Aug 25, 2022

TurkeyMcMac Aug 25, 2022

TurkeyMcMac Aug 25, 2022

sfan5 Aug 25, 2022

TurkeyMcMac Aug 25, 2022

TurkeyMcMac Aug 25, 2022

sfan5 Sep 2, 2022

TurkeyMcMac Sep 2, 2022 •

edited

Loading

sfan5 Sep 13, 2022

TurkeyMcMac Sep 13, 2022

TurkeyMcMac commented Sep 26, 2022

Avoid duplication of mod metadata in memory #12562

Avoid duplication of mod metadata in memory #12562

Conversation

TurkeyMcMac commented Jul 19, 2022 • edited Loading

To do

How to test

Zughy commented Aug 14, 2022

sfan5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TurkeyMcMac Sep 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TurkeyMcMac commented Sep 26, 2022

TurkeyMcMac commented Jul 19, 2022 •

edited

Loading

TurkeyMcMac Sep 2, 2022 •

edited

Loading