Skip to content

Shorten the life cycle of Timeout to get a better gc effect.#4040

Merged
chickenlj merged 1 commit intoapache:masterfrom
carryxyh:refactor-timeout
May 17, 2019
Merged

Shorten the life cycle of Timeout to get a better gc effect.#4040
chickenlj merged 1 commit intoapache:masterfrom
carryxyh:refactor-timeout

Conversation

@carryxyh
Copy link
Copy Markdown
Member

Recently, some pressure tests were performed on the rpc inside the music163. I found that YGC did not perform well during the pressure measurement process. One of the reasons is that the lifetime of TimeoutCheckTask is relatively long (currently it will survive timeout).

I try to shorten its life cycle to get better GC performance. It seems that my modification is successful. Now TimeoutCheckTask will be removed when the request returns.

The purpose of this pr is to submit this optimization item to dubbo.

@carryxyh carryxyh requested review from chickenlj and ralf0131 May 13, 2019 08:50
@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #4040 into master will increase coverage by 0.02%.
The diff coverage is 91.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #4040      +/-   ##
============================================
+ Coverage     63.11%   63.13%   +0.02%     
- Complexity      564      565       +1     
============================================
  Files           753      753              
  Lines         32411    32417       +6     
  Branches       5143     5143              
============================================
+ Hits          20455    20467      +12     
+ Misses         9604     9600       -4     
+ Partials       2352     2350       -2
Impacted Files Coverage Δ Complexity Δ
...dubbo/remoting/exchange/support/DefaultFuture.java 77.77% <91.66%> (+4.3%) 0 <0> (ø) ⬇️
.../remoting/transport/netty4/NettyServerHandler.java 61.9% <0%> (-9.53%) 0% <0%> (ø)
...ng/transport/dispatcher/all/AllChannelHandler.java 51.42% <0%> (-5.72%) 0% <0%> (ø)
.../org/apache/dubbo/remoting/ExecutionException.java 15.78% <0%> (-5.27%) 0% <0%> (ø)
...bo/rpc/cluster/support/FailbackClusterInvoker.java 67.21% <0%> (-3.28%) 0% <0%> (ø)
...pache/dubbo/registry/support/AbstractRegistry.java 78.54% <0%> (-3.07%) 0% <0%> (ø)
...exchange/support/header/HeaderExchangeHandler.java 59.84% <0%> (-2.37%) 0% <0%> (ø)
...he/dubbo/registry/multicast/MulticastRegistry.java 67.87% <0%> (-1.81%) 0% <0%> (ø)
.../exchange/support/header/HeaderExchangeServer.java 66.98% <0%> (-0.95%) 0% <0%> (ø)
...dubbo/rpc/protocol/dubbo/CallbackServiceCodec.java 80.14% <0%> (-0.74%) 0% <0%> (ø)
... and 8 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 e690864...ebdeb37. Read the comment docs.

@chickenlj
Copy link
Copy Markdown
Contributor

LGTM.

TimeoutCheckTask task = new TimeoutCheckTask(future);
TIME_OUT_TIMER.newTimeout(task, future.getTimeout(), TimeUnit.MILLISECONDS);
Timeout t = TIME_OUT_TIMER.newTimeout(task, future.getTimeout(), TimeUnit.MILLISECONDS);
PENDING_TASKS.put(future.getId(), t);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we avoid this PENDING_TASKS instance? I think DefaultFuture itself hosts the status of its timeouttask or TIME_OUT_TIMER hosts the status of all timeout tasks can be better.

I need to merge this PR first to do some pressure test, please help to improve if possible.

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.

3 participants