Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RIP-70-3]Extract adaptive lock mechanism #8663

Merged
merged 65 commits into from
Oct 23, 2024

Conversation

3424672656
Copy link
Contributor

@3424672656 3424672656 commented Sep 7, 2024

Which Issue(s) This PR Fixes

Fixes #8442

Brief Description

How Did You Test This Change?

Test document :https://shimo.im/docs/ZzkLMQ4RwwUa87AQ/

@codecov-commenter
Copy link

codecov-commenter commented Sep 7, 2024

Codecov Report

Attention: Patch coverage is 70.45455% with 39 lines in your changes missing coverage. Please review.

Project coverage is 47.37%. Comparing base (2956f6d) to head (39180a3).

Files with missing lines Patch % Lines
...cketmq/store/lock/AdaptiveBackOffSpinLockImpl.java 71.79% 18 Missing and 4 partials ⚠️
...rg/apache/rocketmq/store/lock/BackOffSpinLock.java 77.14% 6 Missing and 2 partials ⚠️
...ache/rocketmq/store/config/MessageStoreConfig.java 37.50% 5 Missing ⚠️
...main/java/org/apache/rocketmq/store/CommitLog.java 33.33% 0 Missing and 2 partials ⚠️
...e/rocketmq/store/lock/AdaptiveBackOffSpinLock.java 0.00% 2 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #8663      +/-   ##
=============================================
- Coverage      47.39%   47.37%   -0.03%     
- Complexity     11628    11645      +17     
=============================================
  Files           1290     1294       +4     
  Lines          90293    90424     +131     
  Branches       11609    11627      +18     
=============================================
+ Hits           42792    42834      +42     
- Misses         42233    42306      +73     
- Partials        5268     5284      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@fuyou001 fuyou001 left a comment

Choose a reason for hiding this comment

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

CR need

Comment on lines 97 to 114
if (this.adaptiveLock instanceof CollisionRetreatLock) {
CollisionRetreatLock lock = (CollisionRetreatLock) this.adaptiveLock;
int base = Math.min(200 + tps / 200, 500);
if (lock.getNumberOfRetreat(slot) * base >= tps) {
if (lock.isAdapt()) {
lock.adapt(true);
} else {
this.tpsSwapCriticalPoint = tps;
needSwap = true;
}
} else if (lock.getNumberOfRetreat(slot) * base * 3 / 2 <= tps) {
lock.adapt(false);
}
lock.setNumberOfRetreat(slot, 0);
} else {
if (tps <= this.tpsSwapCriticalPoint * 4 / 5) {
needSwap = true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Too many magic numbers, it should be optimized.

import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;

public class CollisionRetreatLock implements AdaptiveBackOffLock {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is RetreatLock? Is it a misspelling of reentrant lock?

private final List<AtomicInteger> numberOfRetreat;

public CollisionRetreatLock() {
this.initOptimalDegree = 1000;
Copy link
Contributor

Choose a reason for hiding this comment

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

You'd better make these numbers as constants, like OPTIMAL_DEGREE = 1000, MAX_OPTIMAL_DEGREE = 10000

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done~


public interface AdaptiveBackOffSpinLock extends PutMessageLock {

void lock();
Copy link
Contributor

Choose a reason for hiding this comment

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

The PutMessageLock class includes methods for locking and unlocking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done~

@@ -130,7 +132,11 @@ protected PutMessageThreadLocal initialValue() {
return new PutMessageThreadLocal(defaultMessageStore.getMessageStoreConfig());
}
};
this.putMessageLock = messageStore.getMessageStoreConfig().isUseReentrantLockWhenPutMessage() ? new PutMessageReentrantLock() : new PutMessageSpinLock();

AdaptiveBackOffSpinLock adaptiveBackOffSpinLock = new AdaptiveBackOffSpinLockImpl();
Copy link
Contributor

Choose a reason for hiding this comment

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

PutMessageLock adaptiveBackOffSpinLock = new AdaptiveBackOffSpinLockImpl() may be better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done~

}
}

if (needSwap) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add some comments to explain the principles and the rationale behind these numbers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done~


public AdaptiveBackOffSpinLockImpl() {
this.locks = new HashMap<>();
this.locks.put("Reentrant", new BackOffReentrantLock());
Copy link
Contributor

Choose a reason for hiding this comment

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

These strings use class constants.

fuyou001
fuyou001 previously approved these changes Oct 19, 2024
… adaptive_lock

# Conflicts:
#	store/src/main/java/org/apache/rocketmq/store/config/MessageStoreConfig.java
Copy link
Contributor

@GenerousMan GenerousMan left a comment

Choose a reason for hiding this comment

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

LGTM

RongtongJin
RongtongJin previously approved these changes Oct 23, 2024
# Conflicts:
#	store/src/main/java/org/apache/rocketmq/store/config/MessageStoreConfig.java
@RongtongJin RongtongJin merged commit 95b88ff into apache:develop Oct 23, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RIP-70] Optimizing Lock Mechanisms
5 participants