Skip to content

[#8377] improvement(core): Optimize CaffeineEntityCache using segmented locking#8452

Merged
yuqi1129 merged 3 commits intoapache:mainfrom
sunxiaojian:issue-8377
Sep 10, 2025
Merged

[#8377] improvement(core): Optimize CaffeineEntityCache using segmented locking#8452
yuqi1129 merged 3 commits intoapache:mainfrom
sunxiaojian:issue-8377

Conversation

@sunxiaojian
Copy link
Collaborator

@sunxiaojian sunxiaojian commented Sep 5, 2025 •
edited
Loading

What changes were proposed in this pull request?

Optimize CaffeineEntityCache using segmented locking

Why are the changes needed?

Fix: #(8377)

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

TestSegmentedLock.java

@sunxiaojian sunxiaojian force-pushed the issue-8377 branch 2 times, most recently from 5f14cf5 to b0be824 Compare September 7, 2025 15:32
@jerryshao jerryshao requested review from xunliu and yuqi1129 September 8, 2025 02:38
yuqi1129
yuqi1129 reviewed Sep 8, 2025
View reviewed changes
core/src/main/java/org/apache/gravitino/Configs.java Outdated
// Number of lock segments for cache concurrency optimization
public static final ConfigEntry<Integer> CACHE_LOCK_SEGMENTS =
new ConfigBuilder("gravitino.cache.lockSegments")
.doc("Number of lock segments for cache concurrency optimization.")
Copy link
Contributor

@yuqi1129 yuqi1129 Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please provide more details about what this configuration entails? You'd better disclose that you are using guava Striped<lock> to replace the traditional lock for better concurrency performance. In the meantime, a reference link is always appreciated.

Copy link
Collaborator Author

@sunxiaojian sunxiaojian Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

core/src/main/java/org/apache/gravitino/Configs.java
.version(ConfigConstants.VERSION_1_0_0)
.intConf()
.checkValue(value -> value > 0, "Lock segments must be positive")
.createWithDefault(16);
Copy link
Contributor

@yuqi1129 yuqi1129 Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is 16 the advised value by guava?

Copy link
Collaborator Author

@sunxiaojian sunxiaojian Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, 16 is not specifically advised by Guava, I referenced the DEFAULT_CAPACITY value of ConcurrentHashMap.

core/src/main/java/org/apache/gravitino/cache/SegmentedLock.java
*/
public <E extends Exception> void withLockAndThrow(
Object key, EntityCache.ThrowingRunnable<E> action) throws E {
waitForGlobalComplete();
Copy link
Contributor

@yuqi1129 yuqi1129 Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't understand why we need to obtain a global lock first and then try to lock the segment lock? Could you give me more detail about it?

Copy link
Collaborator Author

@sunxiaojian sunxiaojian Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yuqi1129 The global lock was intended for clear()

Copy link
Contributor

@yuqi1129 yuqi1129 Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.

@yuqi1129 yuqi1129 mentioned this pull request Sep 9, 2025
Closed
@yuqi1129
Copy link
Contributor

yuqi1129 commented Sep 9, 2025

@sunxiaojian
Please try to solve the CI problem, thanks.

yuqi1129
yuqi1129 reviewed Sep 9, 2025
View reviewed changes
Copy link
Contributor

@yuqi1129 yuqi1129 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It generally LGTM except some minor ones.

@jerryshao
Copy link
Contributor

jerryshao commented Sep 10, 2025

@sunxiaojian We are planning to merge this PR in the 1.0.0, can you please fix the comments mentioned above? If you don't have time, @yuqi1129 can you please help on this?

@jerryshao jerryshao added the branch-1.0 Automatically cherry-pick commit to branch-1.0 label Sep 10, 2025
@sunxiaojian
Copy link
Collaborator Author

sunxiaojian commented Sep 10, 2025

@sunxiaojian We are planning to merge this PR in the 1.0.0, can you please fix the comments mentioned above? If you don't have time, @yuqi1129 can you please help on this?

ok

@yuqi1129
Copy link
Contributor

yuqi1129 commented Sep 10, 2025

Let's wait for the CI pass before proceeding with LGTM.

@yuqi1129 yuqi1129 merged commit 1ab8c05 into apache:main Sep 10, 2025
28 checks passed
github-actions bot pushed a commit that referenced this pull request Sep 10, 2025
@sunxiaojian @web-flow
[#8377] improvement(core): Optimize CaffeineEntityCache using segment…
a2027e0 
…ed locking (#8452)

<!--
1. Title: [#<issue>] <type>(<scope>): <subject>
   Examples:
     - "[#123] feat(operator): support xxx"
     - "[#233] fix: check null before access result in xxx"
     - "[MINOR] refactor: fix typo in variable name"
     - "[MINOR] docs: fix typo in README"
     - "[#255] test: fix flaky test NameOfTheTest"
   Reference: https://www.conventionalcommits.org/en/v1.0.0/
2. If the PR is unfinished, please mark this PR as draft.
-->

### What changes were proposed in this pull request?

Optimize CaffeineEntityCache using segmented locking

### Why are the changes needed?

Fix: [#(8377)](#8377)

### Does this PR introduce _any_ user-facing change?

N/A

### How was this patch tested?

TestSegmentedLock.java
diqiu50 pushed a commit to diqiu50/gravitino that referenced this pull request Sep 15, 2025
@sunxiaojian @diqiu50
[apache#8377] improvement(core): Optimize CaffeineEntityCache using s…
db6c347 
…egmented locking (apache#8452)

<!--
1. Title: [#<issue>] <type>(<scope>): <subject>
   Examples:
     - "[apache#123] feat(operator): support xxx"
     - "[apache#233] fix: check null before access result in xxx"
     - "[MINOR] refactor: fix typo in variable name"
     - "[MINOR] docs: fix typo in README"
     - "[apache#255] test: fix flaky test NameOfTheTest"
   Reference: https://www.conventionalcommits.org/en/v1.0.0/
2. If the PR is unfinished, please mark this PR as draft.
-->

### What changes were proposed in this pull request?

Optimize CaffeineEntityCache using segmented locking

### Why are the changes needed?

Fix: [#(8377)](apache#8377)

### Does this PR introduce _any_ user-facing change?

N/A

### How was this patch tested?

TestSegmentedLock.java
bharos pushed a commit to bharos/gravitino that referenced this pull request Oct 3, 2025
@sunxiaojian @bharos
[apache#8377] improvement(core): Optimize CaffeineEntityCache using s…
40f4f5f 
…egmented locking (apache#8452)

<!--
1. Title: [#<issue>] <type>(<scope>): <subject>
   Examples:
     - "[apache#123] feat(operator): support xxx"
     - "[apache#233] fix: check null before access result in xxx"
     - "[MINOR] refactor: fix typo in variable name"
     - "[MINOR] docs: fix typo in README"
     - "[apache#255] test: fix flaky test NameOfTheTest"
   Reference: https://www.conventionalcommits.org/en/v1.0.0/
2. If the PR is unfinished, please mark this PR as draft.
-->

### What changes were proposed in this pull request?

Optimize CaffeineEntityCache using segmented locking

### Why are the changes needed?

Fix: [#(8377)](apache#8377)

### Does this PR introduce _any_ user-facing change?

N/A

### How was this patch tested?

TestSegmentedLock.java
bharos pushed a commit to bharos/gravitino that referenced this pull request Oct 7, 2025
@sunxiaojian @bharos
[apache#8377] improvement(core): Optimize CaffeineEntityCache using s…
140c4c2 
…egmented locking (apache#8452)

<!--
1. Title: [#<issue>] <type>(<scope>): <subject>
   Examples:
     - "[apache#123] feat(operator): support xxx"
     - "[apache#233] fix: check null before access result in xxx"
     - "[MINOR] refactor: fix typo in variable name"
     - "[MINOR] docs: fix typo in README"
     - "[apache#255] test: fix flaky test NameOfTheTest"
   Reference: https://www.conventionalcommits.org/en/v1.0.0/
2. If the PR is unfinished, please mark this PR as draft.
-->

### What changes were proposed in this pull request?

Optimize CaffeineEntityCache using segmented locking

### Why are the changes needed?

Fix: [#(8377)](apache#8377)

### Does this PR introduce _any_ user-facing change?

N/A

### How was this patch tested?

TestSegmentedLock.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yuqi1129 yuqi1129 yuqi1129 approved these changes

@xunliu xunliu Awaiting requested review from xunliu

Assignees

@sunxiaojian sunxiaojian

Labels

branch-1.0 Automatically cherry-pick commit to branch-1.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Improvement] Optimize CaffeineEntityCache using segmented locking

3 participants

@sunxiaojian @yuqi1129 @jerryshao