Make the cache configurable by cpp API #950

mgautierfr · 2025-02-07T09:39:10Z

This fix #946.

Contrary to what proposed in #946, this doesn't introduce a cache config structure but use simple method:

This follow comment Make cache configuration available from cpp api. #946 (comment)
As we will introduce global cache based on memory, the cluster cache configuration will be move to a plain function.

This PR probably have some small conflict with #949, I will resolve them as soon as a PR is merged.

mgautierfr · 2025-02-07T10:17:47Z

CI seems to be broken for other reasons than this PR.

include/zim/archive.h

veloman-yunkan · 2025-02-10T09:45:56Z

include/zim/archive.h

+      /** Set the size of the dirent lookup cache.
+       *
+       * Contrarly to other `set_<foo>_cache_max_size`, this method is useless once
+       * the lookup cache is created.
+       * The lookup cache is created at first access to a entry in the archive.
+       * So this method must be called before any access to content (including metadata).
+       * It is best to call this method first, just after the archive creation.
+       */
+      void set_dirent_lookup_cache_max_size(size_t new_size);


Isn't it better to through an exception if this method is called when it's too late?

I'm not sure. Another option would be to add a new argument to Archive constructor and remove this function.

src/fileimpl.cpp

test/archive.cpp

src/fileimpl.cpp

src/fileimpl.h

veloman-yunkan

Let's have #949 merged first. I will review the rebased&fixed-up version of this PR again after conflict resolution.

mgautierfr · 2025-02-12T14:31:06Z

PR is now rebased&fixed-up and conflict are resolved.

veloman-yunkan

All my feedback was restricted to the test code, which means that generally the PR is good to merge.

test/lrucache.cpp

test/archive.cpp

mgautierfr · 2025-02-17T14:31:40Z

Last push updates with your comments.
I have directly modified the commits to be ready to merge as soon as you agree (which I hope is soon)

Only test/lru_cache.cpp and test/archive.cpp files have been modified.

veloman-yunkan · 2025-02-17T14:46:36Z

test/archive.cpp

+    if (REF_IT->isRedirect()) {                                                       \
+      ASSERT_EQ(REF_IT->getRedirectEntryIndex(), test_entry.getRedirectEntryIndex()); \
+    }                                                                                 \
+    auto ref_item = REF_IT->getItem(true);                                            \
+    auto test_item = test_entry.getItem(true);                                        \
+    ASSERT_EQ(ref_item.getClusterIndex(), test_item.getClusterIndex());               \
+    ASSERT_EQ(ref_item.getBlobIndex(), test_item.getBlobIndex());                     \
+    ASSERT_EQ(ref_item.getData(), test_item.getData());                               \


Shouldn't the item equivalence testing be in an else branch of the if statement? Shouldn't your tests fail otherwise? Why don't they fail then?

I guess it's because the test archive doesn't contain redirect entries.

´getItem(true)´ resolve redirect(s) and return the final item.
We load the data even if the entry is a redirect to force loading of all cluster.
Else the test fails with withns category at line 366 because only one cluster is present.

veloman-yunkan · 2025-02-17T15:02:49Z

test/archive.cpp

+    ASSERT_ARCHIVE_EQUIVALENT_IT_LIMIT(ref_it, range.end(), archive, 50)
+    EXPECT_EQ(archive.get_dirent_cache_current_size(), 30);
+    EXPECT_EQ(archive.get_cluster_cache_current_size(), 2); // Only 2 clusters in the file
+
+    // Reduce cache size
+    archive.set_dirent_cache_max_size(10);
+    archive.set_cluster_cache_max_size(1);
+
+    EXPECT_EQ(archive.get_dirent_cache_current_size(), 10);
+    EXPECT_EQ(archive.get_cluster_cache_current_size(), 1);
+
+    ASSERT_ARCHIVE_EQUIVALENT_IT_LIMIT(ref_it, range.end(), archive, 50)


This fragment of the test scenario is somewhat obscure and may be misleading. The ASSERT_ARCHIVE_EQUIVALENT_IT_LIMIT() macro has a side effect - it advances its iterator argument. Therefore the second invocation of that macro on the same iterator object continues the iteration (or performs no iteration at all if the end has already been reached before that) and it's not obvious if that is intentional or not. Please add comments explaining what is being tested or change the test accordingly.

Yes, it is intentional, we want to change the cache size as we are iterating on it.
I will add a comment.

veloman-yunkan · 2025-02-17T15:05:50Z

test/archive.cpp

+    // Clean cache
+    // (More than testing the value, this is needed as we want to be sure the cache is actually populated later)
+    archive.set_dirent_cache_max_size(0);
+    archive.set_cluster_cache_max_size(0);
+
+    EXPECT_EQ(archive.get_dirent_cache_current_size(), 0);
+    EXPECT_EQ(archive.get_cluster_cache_current_size(), 0);
+
+    // Increase the cache


With an ASSERT_ARCHIVE_EQUIVALENT() easy/concise check at your service, doesn't it make sense to check that things still work after decreasing the cache size to 0?

The purpose of this test is to test than changing the cache size has an effect. Not that it doesn't change the returned value. ASSERT_ARCHIVE_EQUIVALENT is "just" a nice to have to be sure, but not the main purpose of the test.

The purpose of this test is to test than changing the cache size has an effect.

That's prone to misinterpretation and I also wanted to ask if the commit's description was wrong. Yes, we want to be sure that setting the cache size actually changes the cache size, but I think that we also want to test that changing the cache size on a live archive doesn't break its operation (i.e. in that sense, it has no observable effect other than on performance).

test/archive.cpp

mgautierfr · 2025-02-17T16:01:24Z

I have added a small comment about no reset the ref_it.
Please note the test will kind of become obsolete with PR #956 which introduce a global cluster cache. So I don't it worth have a perfect test here.

- DirentLookup becoming polymorphic - Possibility to disable lookup caching altogether - Once created, this cache size cannot be modified

Probably nobody never set this, and we don't use lzma anymore.

We don't use it anymore.

This make fail the test as it appears we create the dirent lookup cache at initialization. This will be fixed in next commits.

There is no reason this setting is done while reading the mimetype.

…ctor.

Let's use a temp dirent lookup (without cache) to access titleOrdered v1 and 'C' namespace.

Reading and writing in the same time to a unique_ptr is not thread safe. We introduce a atomicBool to know if the unique_ptr has been initialized. Fix #945

mgautierfr requested review from kelson42 and veloman-yunkan February 7, 2025 09:39

mgautierfr force-pushed the cache_config branch from 433e1b4 to 6c5f25e Compare February 7, 2025 10:04

veloman-yunkan requested changes Feb 10, 2025

View reviewed changes

mgautierfr force-pushed the cache_config branch from 6c5f25e to 2c34dbf Compare February 10, 2025 20:35

mgautierfr requested a review from veloman-yunkan February 11, 2025 08:53

veloman-yunkan reviewed Feb 11, 2025

View reviewed changes

mgautierfr force-pushed the cache_config branch from 2c34dbf to e6fbbbf Compare February 12, 2025 14:30

mgautierfr force-pushed the cache_config branch 2 times, most recently from fe9ee9d to 4a1881a Compare February 12, 2025 14:38

veloman-yunkan reviewed Feb 15, 2025

View reviewed changes

test/lrucache.cpp Outdated Show resolved Hide resolved

test/archive.cpp Outdated Show resolved Hide resolved

test/archive.cpp Outdated Show resolved Hide resolved

mgautierfr force-pushed the cache_config branch from 4a1881a to 117868f Compare February 17, 2025 14:28

mgautierfr requested a review from veloman-yunkan February 17, 2025 14:31

veloman-yunkan reviewed Feb 17, 2025

View reviewed changes

mgautierfr force-pushed the cache_config branch 2 times, most recently from 55616af to f3ea99f Compare February 17, 2025 15:59

mgautierfr added 10 commits February 17, 2025 17:01

Introduce drop_last method in lru_cache.

d0de0c1

Introduce helper macro to test cache

3968288

Make lrucache size configurable.

4fd3942

Make concurrent cache size configurable.

8ab9696

Make cluster cache size configurable.

9610f73

Make dirent cache size configurable

ba592e9

Make dirent lookup cache size configurable.

64ff733

- DirentLookup becoming polymorphic - Possibility to disable lookup caching altogether - Once created, this cache size cannot be modified

Do not use env var to limit the maximum size used by lzma.

d945635

Probably nobody never set this, and we don't use lzma anymore.

Remove envvalue source code.

73ad90c

We don't use it anymore.

Add testing of cache

16731d0

mgautierfr added 6 commits February 17, 2025 17:01

Ensure that cache configuration is taken into account

6f4cd59

This make fail the test as it appears we create the dirent lookup cache at initialization. This will be fixed in next commits.

Move setting of UserEntry range out of mimetype.

61793f1

There is no reason this setting is done while reading the mimetype.

Don't hide access to direntLookup in subfonctions in FileImpl constru…

bf0c4de

…ctor.

Don't create the cache dirent lookup at initialization.

d8c7389

Let's use a temp dirent lookup (without cache) to access titleOrdered v1 and 'C' namespace.

Fix potential race condition on m_direntLookup initialization.

fa21419

Reading and writing in the same time to a unique_ptr is not thread safe. We introduce a atomicBool to know if the unique_ptr has been initialized. Fix #945

Correctly test that changing the cache size on live archive has effect.

cc02757

mgautierfr force-pushed the cache_config branch from f3ea99f to cc02757 Compare February 17, 2025 16:02

veloman-yunkan approved these changes Feb 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the cache configurable by cpp API #950

Make the cache configurable by cpp API #950

mgautierfr commented Feb 7, 2025

mgautierfr commented Feb 7, 2025

veloman-yunkan Feb 10, 2025

mgautierfr Feb 10, 2025

veloman-yunkan left a comment

mgautierfr commented Feb 12, 2025

veloman-yunkan left a comment

mgautierfr commented Feb 17, 2025

veloman-yunkan Feb 17, 2025

veloman-yunkan Feb 17, 2025

mgautierfr Feb 17, 2025

veloman-yunkan Feb 17, 2025

mgautierfr Feb 17, 2025 •

edited

Loading

veloman-yunkan Feb 17, 2025 •

edited

Loading

mgautierfr Feb 17, 2025

veloman-yunkan Feb 17, 2025

mgautierfr commented Feb 17, 2025 •

edited

Loading

Make the cache configurable by cpp API #950

Are you sure you want to change the base?

Make the cache configurable by cpp API #950

Conversation

mgautierfr commented Feb 7, 2025

mgautierfr commented Feb 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veloman-yunkan left a comment

Choose a reason for hiding this comment

mgautierfr commented Feb 12, 2025

veloman-yunkan left a comment

Choose a reason for hiding this comment

mgautierfr commented Feb 17, 2025

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

mgautierfr Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

veloman-yunkan Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgautierfr commented Feb 17, 2025 • edited Loading

mgautierfr Feb 17, 2025 •

edited

Loading

veloman-yunkan Feb 17, 2025 •

edited

Loading

mgautierfr commented Feb 17, 2025 •

edited

Loading