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

feat: add MemorySafeLinkedBlockingQueue #1213

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Codeprh
Copy link

@Codeprh Codeprh commented Jun 10, 2022

Motivation:

Can completely solve the OOM problem caused by {@link java.util.concurrent.LinkedBlockingQueue}

Modification and Result:

CN

参考dubbo的思想(dubbo-pr),引入MemorySafeLinkedBlockingQueue,解决无界队列可能会导致OOM的问题

EN

Referring to the idea of dubbo(dubbo-pr), introduce MemorySafeLinkedBlockingQueue to solve the problem that unbounded queues may cause OOM

@sofastack-bot
Copy link

sofastack-bot bot commented Jun 10, 2022

Hi @Codeprh, welcome to SOFAStack community, Please sign Contributor License Agreement!

After you signed CLA, we will automatically sync the status of this pull request in 3 minutes.

@sofastack-bot sofastack-bot bot added cla:no Need sign CLA First-time contributor First-time contributor size/L labels Jun 10, 2022
@EvenLjj
Copy link
Collaborator

EvenLjj commented Jun 24, 2022

Hi @Codeprh, thank for your PR. Please run mvn clean compile -DskipTests before you commit to format the code.
image

@Codeprh
Copy link
Author

Codeprh commented Jul 5, 2022

mvn clean compile -DskipTests

image

I'm done, I think there is no problem with my code, please help me, thank you~

@sofastack-bot sofastack-bot bot added cla:yes CLA is ok and removed cla:no Need sign CLA labels Jul 6, 2022
@Codeprh
Copy link
Author

Codeprh commented Jul 6, 2022

Hi @Codeprh, thank for your PR. Please run mvn clean compile -DskipTests before you commit to format the code. image

done,3q

@codecov
Copy link

codecov bot commented Jul 9, 2022

Codecov Report

Merging #1213 (ae2e21f) into master (bb7c51b) will increase coverage by 0.52%.
The diff coverage is 56.25%.

@@             Coverage Diff              @@
##             master    #1213      +/-   ##
============================================
+ Coverage     71.56%   72.09%   +0.52%     
+ Complexity      832      780      -52     
============================================
  Files           408      412       +4     
  Lines         17224    17403     +179     
  Branches       2684     2702      +18     
============================================
+ Hits          12327    12546     +219     
+ Misses         3533     3475      -58     
- Partials       1364     1382      +18     
Impacted Files Coverage Δ
...mmon/threadpool/MemorySafeLinkedBlockingQueue.java 50.00% <50.00%> (ø)
...a/rpc/common/threadpool/MemoryLimitCalculator.java 61.53% <61.53%> (ø)
.../alipay/sofa/rpc/common/utils/ThreadPoolUtils.java 95.00% <100.00%> (ø)
...ofa/rpc/registry/zk/ZookeeperProviderObserver.java 72.50% <0.00%> (-2.50%) ⬇️
...a/com/alipay/sofa/rpc/event/LookoutSubscriber.java 88.63% <0.00%> (-2.28%) ⬇️
.../alipay/sofa/rpc/metrics/lookout/RpcLookoutId.java 87.30% <0.00%> (-1.59%) ⬇️
...lipay/sofa/rpc/message/AbstractResponseFuture.java 56.14% <0.00%> (-0.88%) ⬇️
.../java/com/alipay/sofa/rpc/common/RpcConstants.java 100.00% <0.00%> (ø)
...alipay/sofa/rpc/registry/local/DomainRegistry.java 94.11% <0.00%> (ø)
.../sofa/rpc/registry/local/DomainRegistryHelper.java 62.50% <0.00%> (ø)
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb7c51b...ae2e21f. Read the comment docs.


@Override
public void put(final E e) throws InterruptedException {
if (hasRemainedMemory()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is important to notify users when they fail to put an element to the queue.

Copy link

@loongs-zhang loongs-zhang Jul 27, 2022

Choose a reason for hiding this comment

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

Good idea. Maybe notify action should like java.util.concurrent.RejectedExecutionHandler.

@@ -248,7 +250,7 @@ public static BlockingQueue<Runnable> buildQueue(int size, boolean isPriority) {
queue = size < 0 ? new PriorityBlockingQueue<Runnable>()
: new PriorityBlockingQueue<Runnable>(size);
} else {
queue = size < 0 ? new LinkedBlockingQueue<Runnable>()
queue = size < 0 ? new MemorySafeLinkedBlockingQueue<Runnable>()
: new LinkedBlockingQueue<Runnable>(size);
Copy link

Choose a reason for hiding this comment

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

shall be an ArrayBlockingQueue once size is set. LinkedBlockingQueue will not block/reject adding a new element when reaching threshold(input size).

But it's Ok not to fix this, as it is what it was.

@OrezzerO
Copy link
Contributor

also see : apache/dubbo#9789

@loongs-zhang
Copy link

@OrezzerO @Codeprh How about this PR: apache/shenyu#3764 ?

@Codeprh Codeprh requested a review from OrezzerO July 31, 2022 14:13
@JervyShi
Copy link
Member

JervyShi commented Aug 9, 2022

@OrezzerO @Codeprh How about this PR: apache/shenyu#3764 ?

This PR has better handling of exceptions. 👍

@EvenLjj EvenLjj added this to the 5.9.0 milestone Sep 13, 2022
@stale
Copy link

stale bot commented Nov 12, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Nov 12, 2022
@EvenLjj EvenLjj added remind To be further discussed and removed wontfix This will not be worked on labels Nov 13, 2022
@nobodyiam
Copy link
Member

review again

Copy link
Member

nobodyiam commented May 5, 2023

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.


Overall summary:

This pull request introduces the "feat: add MemorySafeLinkedBlockingQueue" feature, which involves the creation of the MemoryLimitCalculator class and modifications to the MemorySafeLinkedBlockingQueue, MemoryLimiter, ThreadPoolUtils, and related test files. The primary goal of these changes is to address the OutOfMemory (OOM) problem caused by java.util.concurrent.LinkedBlockingQueue.

Potential issues:

  1. NullPointerException might be thrown during object acquisition in the MemoryLimiter class. Proper input validation should be performed.
  2. Ensure that the MemoryLimitCalculator's scheduled refresh of maximum available memory does not cause performance issues.
  3. Verify concurrency, performance, and accuracy of computation of memory usage in MemoryLimiter.
  4. Check the test cases included in MemorySafeLinkedBlockingQueueTest to confirm adequate testing coverage and reliability.
  5. Ensure the proper removal of the MemoryLimiter class and verify if there are any remaining dependencies on this class.
  6. Ensure that the OOM problem is completely solved by the changes made to the MemoryLimitCalculator class.

Key findings:

  1. The MemoryLimitCalculator class has been created for calculating and periodically refreshing maximum available memory.
  2. The MemoryLimiter class is no longer needed and has been removed, but its removal's impact on the codebase should be double-checked.
  3. The MemorySafeLinkedBlockingQueue class has been created as an implementation of the queue using the MemoryLimitCalculator.
  4. Test cases have been added and modified, but it is important to ensure that adequate testing coverage and reliability is maintained for the new changes.

Recommendations:

  1. Check for any remaining dependencies on the MemoryLimiter class and ensure that its removal does not cause any integration issues.
  2. Thoroughly test the changes made to the MemoryLimitCalculator class to confirm that the OOM problem is indeed solved under different scenarios.
  3. Make sure that there is proper locking and unlocking for acquiring or releasing objects in memory, and no thread safety issues are present.
  4. Verify that the testing coverage is adequate, especially for the new changes in the MemorySafeLinkedBlockingQueue file.

Details

Commit b5fd13d458ba25ab08579a5b5149c0a1d80ea83f

Summary of key changes:

  1. Creation of MemoryLimitCalculator class to calculate and periodically refresh the maximum available memory.
  2. Creation of MemoryLimiter class to handle acquiring and releasing objects in memory.
  3. Creation of MemorySafeLinkedBlockingQueue class, which is a queue implementation that uses the MemoryLimiter.
  4. Added a new test for MemorySafeLinkedBlockingQueue (MemorySafeLinkedBlockingQueueTest).
  5. Minor update in ThreadPoolUtils class.

Key findings and potential problems:

  1. NullPointerException might be thrown during object acquisition in the MemoryLimiter class. Ensure that proper input validation is performed.
  2. Ensure that the MemoryLimitCalculator's scheduled refresh of maximum available memory does not cause performance issues.
  3. While acquiring or releasing objects in memory, ensure that proper locking and unlocking are performed, and no thread safety issues are present.
  4. Verify concurrency, performance, and accuracy of computation of memory usage in MemoryLimiter.
  5. Check the test cases included in MemorySafeLinkedBlockingQueueTest to confirm adequate testing coverage and reliability.

Commit 94e41ca93544c57a864a64a76277efb7914e595e

This pull request makes changes to 4 files related to the MemorySafeLinkedBlockingQueue implementation:

Key changes:

  1. The MemoryLimitCalculator.java file has only a line removed which doesn't seem to impact any functionality.
  2. The MemoryLimiter.java file had some formatting changes (e.g., adjustments to field alignment), but no changes to the logic.
  3. The key changes are in the implementation of the MemorySafeLinkedBlockingQueue.java file, where the static variable THE_256_MB is set to 256MB, and the constructor with no arguments initializes the instance variable maxFreeMemory with the value of THE_256_MB.
  4. The MemorySafeLinkedBlockingQueueTest.java file has new license comment block and additional lines of whitespace but no changes in the unit tests.

Potential problems:

  1. No significant updates to the testing coverage to account for new changes in the MemorySafeLinkedBlockingQueue.java file.

Commit 622ba0a251513b3d54a25ce31135fab09c21fc08

This patch is for the file ThreadPoolUtilsTest.java and modifies the test cases for the buildQueue and buildQueue1 methods. The key changes are:

  1. Import statement for MemorySafeLinkedBlockingQueue has been added.
  2. Two assertions are updated to expect the MemorySafeLinkedBlockingQueue class instead of the LinkedBlockingQueue class for the cases with a capacity of -1 in both buildQueue and buildQueue1 test methods.

Potential problems:

  1. The actual implementation of MemorySafeLinkedBlockingQueue or its integration with ThreadPoolUtils is not present in this patch. Ensure that these changes are present in the complete Pull Request or other related patches.
  2. The test suite may lack sufficient testing or edge cases for the MemorySafeLinkedBlockingQueue implementation, depending on its complexity and possible side effects on the ThreadPoolUtils.

Commit ae2e21f554e0f7317c7c73eedfd245be769403b1

Summary of key changes in the patch:

  1. Removal of MemoryLimiter class (core/common/src/main/java/com/alipay/sofa/rpc/common/threadpool/MemoryLimiter.java).
  2. Modification of the MemoryLimitCalculator class
    • Removal of its dependency on MemoryLimiter.
    • Update of JavaDoc to mention it can completely solve the OOM problem caused by java.util.concurrent.LinkedBlockingQueue and does not depend on java.lang.instrument.Instrumentation.

Potential problems:

  1. The removal of the MemoryLimiter class can have a substantial impact on the codebase, and it's essential to ensure that there are no dependencies on this class. As it's not clear whether or not other parts of the code still depend on MemoryLimiter, it's essential to check for any possible integration issues.

  2. While the updated JavaDoc for MemoryLimitCalculator mentions that it can completely solve the OOM problem, it's critical to test the code under different scenarios where an OOM problem could occur, ensuring its correctness and effectiveness. This is important because solving the OOM problem is one of the main goals of this pull request.

Based on this patch, I recommend checking for any dependencies on the MemoryLimiter class and making sure that the supposed OOM problem is indeed solved by the changes made to the MemoryLimitCalculator class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes CLA is ok First-time contributor First-time contributor remind To be further discussed size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants