Cache project metadata and tokens #1099

minwoox · 2025-02-14T14:46:45Z

Motivation:
Currently, only the raw contents of ProjectMetadata and Tokens are cached when fetched from MetadataService. However, deserialization takes a significant amount of time, so caching the deserialized instances would improve performance.

Modifications:

Introduced CacheableCall interface.
Added Repository.execute(CacheableCall) to cache calls.
Cached deserialized instances of ProjectMetadata and Tokens retrieved by MetadataService.
Remove stripe locks that were used for CacheableCompareTreesCall.

Result:

Improved performance by avoiding redundant deserialization of ProjectMetadata and Tokens.

Motivation: Currently, only the raw contents of `ProjectMetadata` and `Tokens` are cached when fetched from `MetadataService`. However, deserialization takes a significant amount of time, so caching the deserialized instances would improve performance. Modifications: - Renamed `CacheableCall` to `AbstractCacheableCall`. - Introduced `CacheableCall` interface. - Added `Repository.execute(CacheableCall)` to cache calls. - Cached deserialized instances of `ProjectMetadata` and `Tokens` retrieved by `MetadataService`. Result: - Improved performance by avoiding redundant deserialization of `ProjectMetadata` and `Tokens`.

ikhoon · 2025-02-18T02:19:11Z

server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java

-                                               entry -> convertWithJackson(entry, ProjectMetadata.class));
-        tokenRepo = new RepositorySupport<>(projectManager, executor,
-                                            entry -> convertWithJackson(entry, Tokens.class));
+        metadataRepo = new RepositorySupport<>(projectManager, executor, ProjectMetadata.class);


In fact, ProjectMetadata is already cached in DefaultProject and automatically refreshed when the raw data is updated.

What do you think of replacing the implementation of fetchMetadata0() with projectManager.get(projectName).metadata()?

I can't replace it completely because fetching is still used here:

centraldogma/server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java

Line 170 in b9cebef

return fetchMetadata0(projectName);

Let me add getMetdata instead. 😉

server/src/main/java/com/linecorp/centraldogma/server/metadata/Roles.java

ikhoon · 2025-02-18T02:41:56Z

...va/com/linecorp/centraldogma/server/internal/storage/repository/cache/CachingRepository.java

+        return unsafeCast(cache.get(cacheableCall).handleAsync((unused, cause) -> {
+            throwUnsafelyIfNonNull(cause);
+            return unused;


It doesn't seem like that unused is a proper name.

Suggested change

return unsafeCast(cache.get(cacheableCall).handleAsync((unused, cause) -> {

throwUnsafelyIfNonNull(cause);

return unused;

return unsafeCast(cache.get(cacheableCall).handleAsync((value, cause) -> {

throwUnsafelyIfNonNull(cause);

return value;

ikhoon · 2025-02-18T03:10:42Z

.../main/java/com/linecorp/centraldogma/server/internal/storage/repository/RepositoryCache.java

-                // Cached already.
-                return existingValue;
-            }
+        final CompletableFuture<T> future = new CompletableFuture<>();


Question) Should we check if the cache was populated by a previous call first before creating a new future?

Creating a future is cheaper than calling Cache.getIfPresent.

This approach is recommended by the team. (Sorry for not giving you enough information.)
https://github.com/ben-manes/caffeine/wiki/Faq#recursive-computations

I would like to know what your reasoning is. Could you elaborate on that? Is .getIfPresent more expensive cost than .putIfAbsent?

If you want to check if the cache was popolulated, you need to call getIfPresent first and you need to call put or putIfAbsent if it's not created.
But in this approach, we can just call putIfAbsent only once. Did I miss something?

I understand your intention. There is a trade-off for that. If the cache hit ratio is high, most calls will be handled by getIfPresent().

As the change is trivial, let's keep it as it. I just wondered the reason. :-)

That is also true.
Creating a future is relatively cheap and the code is shorter so I prefer this way. 😉

ikhoon · 2025-02-18T03:23:47Z

server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java

-                                               entry -> convertWithJackson(entry, ProjectMetadata.class));
-        tokenRepo = new RepositorySupport<>(projectManager, executor,
-                                            entry -> convertWithJackson(entry, Tokens.class));
+        metadataRepo = new RepositorySupport<>(projectManager, executor, ProjectMetadata.class);


What do you think of replacing the implementation of fetchMetadata0() with projectManager.get(projectName).metadata()?

ikhoon · 2025-02-18T03:30:30Z

Suggestion) It would be worth caching Tokens in a strong reference so we don't lose the cache.
How about caching Tokens using RepositoryListener? We may remove some costs such as creating CompletableFuture, hash lookup, and cache miss.

minwoox · 2025-02-18T03:34:21Z

Suggestion) It would be worth caching Tokens in a strong reference so we don't lose the cache.

That's a good suggestion. Let me handle it in this PR. 👍

minwoox · 2025-02-18T03:50:06Z

Suggestion) It would be worth caching Tokens in a strong reference so we don't lose the cache.

@ikhoon MetadataService is created multiple places so I need some refactoring. Let me create a separate PR for that if you don't mind.

ikhoon · 2025-02-18T06:40:44Z

Let me create a separate PR for that if you don't mind.

Let's do that.

ikhoon

LGTM once the CI builds pass. 👍

minwoox · 2025-02-18T14:05:26Z

server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java


    private final Map<String, CompletableFuture<Revision>> reposInAddingMetadata = new ConcurrentHashMap<>();

    /**
     * Creates a new instance.
     */
-    public MetadataService(ProjectManager projectManager, CommandExecutor executor) {
+    public MetadataService(ProjectManager projectManager, CommandExecutor executor,


Please reveiew this class with the hiding whitespace mode. 😉

minwoox · 2025-02-18T14:11:22Z

MetadataService is created multiple places so I need some refactoring. Let me create a separate PR for that if you don't mind.

I put the patches in this PR. 😓
The Tokens are cached and retrieved via InternalProjectInitializer.
Uncached tokens are also fetched when the TokenService needs to retrieve the tokens right away after adding, updating and deleting a token. PTAL.

minwoox · 2025-02-19T01:10:25Z

server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/TokenService.java

        if (loginUser.isSystemAdmin()) {
-            return mds.getTokens()
-                      .thenApply(tokens -> tokens.appIds().values());
+            return mds.getTokens().appIds().values();


We can simply use the cached tokens in this case.

minwoox · 2025-02-19T01:11:25Z

server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/TokenService.java

@@ -127,12 +123,12 @@ public CompletableFuture<HttpResult<Token>> createToken(@Param String appId,
            tokenFuture = mds.createToken(author, appId, isSystemAdminToken);
        }
        return tokenFuture
-                .thenCompose(unused -> mds.findTokenByAppId(appId))
+                .thenCompose(unused -> fetchTokensByAppId(appId))


We have to fetch the tokens because the tokens are just updated.

jrhee17

Looked through the code, and the direction looks good to me.

I didn't check cases where a create directly leads to a get - hopefully the following comment is addressed before RepositoryListener is used more extensively
ref: #1053 (comment)

minwoox added the improvement label Feb 14, 2025

minwoox added this to the 0.74.0 milestone Feb 14, 2025

minwoox requested review from ikhoon, jrhee17 and trustin as code owners February 14, 2025 14:46

minwoox added 3 commits February 14, 2025 23:52

Fix

33a6e3f

Fix

6e0a118

Fix

823e4ad

jrhee17 approved these changes Feb 18, 2025

View reviewed changes

ikhoon reviewed Feb 18, 2025

View reviewed changes

Address the comments from @ikhoon

7856455

ikhoon approved these changes Feb 18, 2025

View reviewed changes

Use IntetnalProjectInitializer for tokens

4407579

minwoox commented Feb 18, 2025

View reviewed changes

Remove cruft

a7b1752

Fix flaky

9d82d54

minwoox commented Feb 19, 2025

View reviewed changes

Fix

2b0860f

minwoox requested review from jrhee17 and ikhoon February 20, 2025 09:45

jrhee17 approved these changes Feb 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache project metadata and tokens #1099

Cache project metadata and tokens #1099

minwoox commented Feb 14, 2025 •

edited

Loading

ikhoon Feb 18, 2025

ikhoon Feb 18, 2025

minwoox Feb 18, 2025

ikhoon Feb 18, 2025

ikhoon Feb 18, 2025

minwoox Feb 18, 2025 •

edited

Loading

minwoox Feb 18, 2025

ikhoon Feb 18, 2025

minwoox Feb 18, 2025

ikhoon Feb 18, 2025

minwoox Feb 18, 2025

ikhoon Feb 18, 2025

ikhoon commented Feb 18, 2025

minwoox commented Feb 18, 2025

minwoox commented Feb 18, 2025

ikhoon commented Feb 18, 2025

ikhoon left a comment

minwoox Feb 18, 2025

minwoox commented Feb 18, 2025

minwoox Feb 19, 2025

minwoox Feb 19, 2025

jrhee17 left a comment

Cache project metadata and tokens #1099

Are you sure you want to change the base?

Cache project metadata and tokens #1099

Conversation

minwoox commented Feb 14, 2025 • edited Loading

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

minwoox Feb 18, 2025 • edited Loading

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

ikhoon commented Feb 18, 2025

minwoox commented Feb 18, 2025

minwoox commented Feb 18, 2025

ikhoon commented Feb 18, 2025

ikhoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox commented Feb 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

minwoox commented Feb 14, 2025 •

edited

Loading

minwoox Feb 18, 2025 •

edited

Loading