HDDS-9912. EC client Reusing thread resources #5791

xichen01 · 2023-12-14T09:10:58Z

What changes were proposed in this pull request?

When writing an EC key, multiple temporary threads are created, creating and destroying these threads will cause performance degradation, this PR improves the performance of writing an EC key by reusing thread resources.

The Root cause

For a 3 + 2 EC key 1 flushExecutor thread and 5 responseExecutors are created.

For a key in replica mode, only one responseExecutor is created, because replica mode only needs to write once and the rest of the replicas are synchronized by Ratis, whereas EC mode requires the Client to write all the strips to DNs.

So temporary threads have less impact on performance in replica mode buckets, and more impact in EC buckets.

ozone/hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java

Line 178 in 8b25c55

responseExecutor = Executors.newSingleThreadExecutor();

ozone/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/ECKeyOutputStream.java

Line 159 in 8b25c55

this.flushExecutor = Executors.newSingleThreadExecutor();

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-9912

How was this patch tested?

Existing test
Performance testing result
https://issues.apache.org/jira/browse/HDDS-9911

# Conflicts: # hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java # hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ECBlockOutputStream.java # hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/RatisBlockOutputStream.java # hadoop-hdds/client/src/test/java/org/apache/hadoop/hdds/scm/storage/TestBlockOutputStreamCorrectness.java # hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ec/reconstruction/ECReconstructionCoordinator.java # hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/BlockOutputStreamEntry.java # hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/BlockOutputStreamEntryPool.java # hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/ECBlockOutputStreamEntry.java # hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/ECBlockOutputStreamEntryPool.java # hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/ECKeyOutputStream.java # hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java # hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java # hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneOutputStreamStub.java

kerneltime · 2023-12-18T17:17:27Z

cc @tanvipenumudy

adoroszlai · 2024-01-10T21:32:13Z

@duongkame @jojochuang @kerneltime @tanvipenumudy @umamaheswararao please review

jojochuang

LGTM not that familiar with EC but the analysis and the code makes sense to me.

Great work!

adoroszlai · 2024-01-14T13:35:40Z

hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java

@@ -2358,7 +2366,7 @@ private KeyOutputStream.Builder createKeyOutputStream(
        .enableUnsafeByteBufferConversion(unsafeByteBufferConversion)
        .setConfig(clientConfig)
        .setAtomicKeyCreation(isS3GRequest.get())
-        .setClientMetrics(clientMetrics)
+        .setBlockOutPutStreamResourceProvider(blockOutPutStreamResourceProvider)


Maybe I'm missing something, but it seems the same ecWriteExecutor is used for both EC and Ratis keys:

ozone/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java

Lines 319 to 320 in f695a37

this.blockOutPutStreamResourceProvider = BlockOutPutStreamResourceProvider

.create(this::getECWriteThreadPool, ContainerClientMetrics.acquire());

@xichen01 Shouldn't we use a different resource provider for Ratis keys?

Hi @adoroszlai @xichen01 , Thanks work on this, I think the reason for the current SingleThreadExecutor is because it uses asynchrony in an OutputStream and needs to guarantee the order of execution. If that's the case, is switching to a multi-threaded Executor still a guarantee?

Maybe I'm missing something, but it seems the same ecWriteExecutor is used for both EC and Ratis keys:

ozone/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java

Lines 319 to 320 in f695a37

this.blockOutPutStreamResourceProvider = BlockOutPutStreamResourceProvider

.create(this::getECWriteThreadPool, ContainerClientMetrics.acquire());

@xichen01 Shouldn't we use a different resource provider for Ratis keys?

@adoroszlai Yes, since Ratis Key and EC Key reuse class BlockOutputStream, the EC Key and Ratis Key both use this ecWriteExecutor. I think that's acceptable, since ecWriteExecutor just provides thread resources, not logic. And the Ratis key also benefits from not having to create a new thread for each Block. But mybe we need rename the ecWriteExecutor to a more normal name.

Thanks for the clarification. I agree, we should rename it if it's intended for both.

Hi @adoroszlai @xichen01 , Thanks work on this, I think the reason for the current SingleThreadExecutor is because it uses asynchrony in an OutputStream and needs to guarantee the order of execution. If that's the case, is switching to a multi-threaded Executor still a guarantee?

@guohao-rosicky. I think it's okay here, because ecWriteExecutor is just used to wait for an RPC response, not to send a request. the thread that sends request was always a fixed single thread.
@adoroszlai how do you think?

Thanks a lot @xichen01 for updating the patch. Most of the updates look good, but I think this part needs to improved. I think it would be better to replace the LinkedList in AbstractCommitWatcher with a CopyOnWriteArrayList, rather than synchronizing thenApplyAsync completely.

CC @szetszwo

Could the synchronized be moved to AbstractCommitWatcher? It is a much smaller class.

I noticed that the closure accesses a lot of external variables, which could be a potential thread-safety issue, so I just added a "big" synchronized here, which ensures that even if someone changes the code later on, there's probably no thread-safety issue, and the thenApplyAsync just sets/creates the variable, which is supposed to be lightweight.
So, I think the current approach is acceptable, what do you think?

closure accesses a lot of external variables, which could be a potential thread-safety issue

I have the following concerns:

Are the same external variables protected by the same object elsewhere? If not, there is no point in making this synchronized.

validateResponse only accesses an atomic reference and the response object itself. Neither of those need to be synced.

Protobuf conversion (BlockID.getFromProtobuf) should not be performed while holding the lock.

(this.)blockID is also atomic, does not need the lock.

logging should not be performed while holding the lock.

So in the end I think only updateCommitInfo needs to be protected.

synchronized has been moved to AbstractCommitWatcher.

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/OzoneClientConfig.java

hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java

...lient/src/main/java/org/apache/hadoop/ozone/client/io/BlockOutPutStreamResourceProvider.java

hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/ECKeyOutputStream.java

...lient/src/main/java/org/apache/hadoop/ozone/client/io/BlockOutPutStreamResourceProvider.java

szetszwo · 2024-02-05T18:28:05Z

@xichen01 , thanks a lot for working on this. The idea of reusing threads is great! The change here is quite big. How about separating this into at least two JIRAs?

changing both the common code
changing the ec code.

xichen01 · 2024-02-06T11:10:00Z

@xichen01 , thanks a lot for working on this. The idea of reusing threads is great! The change here is quite big. How about separating this into at least two JIRAs?

changing both the common code

changing the ec code.

I understand, but although the code seems to have changed a lot, most of the changes were actually made to pass parameters, so it may not be very easy to split, but if you think it's necessary, I'll give it a try.

# Conflicts: # hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ec/reconstruction/ECReconstructionCoordinator.java

szetszwo · 2024-02-06T18:07:17Z

I understand, but although the code seems to have changed a lot, most of the changes were actually made to pass parameters, so it may not be very easy to split, but if you think it's necessary, I'll give it a try.

I tried to review this but not able to reason about the changes. In particular, I like to understand why it needs the new class BlockOutputStreamResourceProvider and also the synchronization change.

If the changes are just some simply, mechanical changes, it is not hard to review even the change size is large. However, it is hard to review when some complicated changes (e.g. synchronization) is mixed in a lot of simple changes.

xichen01 · 2024-02-09T11:14:15Z

I understand, but although the code seems to have changed a lot, most of the changes were actually made to pass parameters, so it may not be very easy to split, but if you think it's necessary, I'll give it a try.

I tried to review this but not able to reason about the changes. In particular, I like to understand why it needs the new class BlockOutputStreamResourceProvider and also the synchronization change.

If the changes are just some simply, mechanical changes, it is not hard to review even the change size is large. However, it is hard to review when some complicated changes (e.g. synchronization) is mixed in a lot of simple changes.

OK, thank for you explain, I will try to split this into two JIRAs.

xichen01 · 2024-02-15T10:18:13Z

Split this PR to:
https://issues.apache.org/jira/browse/HDDS-10383
https://issues.apache.org/jira/browse/HDDS-10384

szetszwo · 2024-02-15T16:29:21Z

@xichen01 , thank a lot! Will review both JIRAs.

HDDS-9912. EC client Reusing thread resources

c203130

adoroszlai added the performance label Dec 14, 2023

xichen01 added 3 commits December 16, 2023 23:48

Fix test

a1d8630

Fix conflicts

7e808b4

kerneltime requested review from duongkame, kerneltime, umamaheswararao and jojochuang December 18, 2023 17:17

Fix test

f695a37

jojochuang approved these changes Jan 10, 2024

View reviewed changes

adoroszlai reviewed Jan 14, 2024

View reviewed changes

adoroszlai reviewed Jan 30, 2024

View reviewed changes

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/OzoneClientConfig.java Outdated Show resolved Hide resolved

hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java Outdated Show resolved Hide resolved

xichen01 and others added 2 commits January 31, 2024 00:08

Rename ecWriteExecutor

052843b

Merge remote-tracking branch 'origin/master' into HDDS-9912

1d0f7ca

adoroszlai reviewed Feb 2, 2024

View reviewed changes

xichen01 added 4 commits February 3, 2024 01:45

Make thread safe

1a49dec

Make writeExecutor unbounded

cf4372a

rename BlockOutPutStreamResourceProvider

78b40ab

Merge branch 'HDDS-9912' of github.com:xichen01/ozone into HDDS-9912

2ab6848

xichen01 added 3 commits February 6, 2024 19:11

Merge branch 'master' into HDDS-9912

08f9482

Merge branch 'master' into HDDS-9912

2ae2f25

# Conflicts: # hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ec/reconstruction/ECReconstructionCoordinator.java

Fix conflict

567496d

Reduce synchronized granularity.

5769cbd

xichen01 closed this Feb 15, 2024

xichen01 mentioned this pull request Feb 15, 2024

HDDS-10383. Introduce a Provider for client-side thread resources passing #6222

Merged

xichen01 mentioned this pull request Feb 24, 2024

HDDS-10384. RPC client Reusing thread resources. #6270

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HDDS-9912. EC client Reusing thread resources #5791

HDDS-9912. EC client Reusing thread resources #5791

xichen01 commented Dec 14, 2023 •

edited

Loading

kerneltime commented Dec 18, 2023

adoroszlai commented Jan 10, 2024 •

edited

Loading

jojochuang left a comment •

edited

Loading

adoroszlai Jan 14, 2024 •

edited

Loading

guohao-rosicky Jan 29, 2024

xichen01 Jan 29, 2024

adoroszlai Jan 29, 2024 •

edited

Loading

xichen01 Jan 30, 2024

adoroszlai Feb 3, 2024

szetszwo Feb 5, 2024

xichen01 Feb 6, 2024

adoroszlai Feb 6, 2024

xichen01 Feb 9, 2024

szetszwo commented Feb 5, 2024

xichen01 commented Feb 6, 2024

szetszwo commented Feb 6, 2024

xichen01 commented Feb 9, 2024

xichen01 commented Feb 15, 2024

szetszwo commented Feb 15, 2024

	this.blockOutPutStreamResourceProvider = BlockOutPutStreamResourceProvider
	.create(this::getECWriteThreadPool, ContainerClientMetrics.acquire());

HDDS-9912. EC client Reusing thread resources #5791

HDDS-9912. EC client Reusing thread resources #5791

Conversation

xichen01 commented Dec 14, 2023 • edited Loading

What changes were proposed in this pull request?

The Root cause

What is the link to the Apache JIRA

How was this patch tested?

kerneltime commented Dec 18, 2023

adoroszlai commented Jan 10, 2024 • edited Loading

jojochuang left a comment • edited Loading

Choose a reason for hiding this comment

adoroszlai Jan 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adoroszlai Jan 29, 2024 • 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

szetszwo commented Feb 5, 2024

xichen01 commented Feb 6, 2024

szetszwo commented Feb 6, 2024

xichen01 commented Feb 9, 2024

xichen01 commented Feb 15, 2024

szetszwo commented Feb 15, 2024

xichen01 commented Dec 14, 2023 •

edited

Loading

adoroszlai commented Jan 10, 2024 •

edited

Loading

jojochuang left a comment •

edited

Loading

adoroszlai Jan 14, 2024 •

edited

Loading

adoroszlai Jan 29, 2024 •

edited

Loading