Various performance optimization for CRT sync HTTP client #4776
Conversation
| @@ -125,6 +128,11 @@ public int read(byte[] bytes, int off, int len) { | |||
| @Override | |||
| public void close() { | |||
| synchronized (subscribeLock) { | |||
| // If it is done, no-op | |||
| if (inputStreamState.get().equals(State.STREAMING_DONE)) { | |||
There was a problem hiding this comment.
The SDK will close the input stream once a request is finished and in this case, if we already have all data buffered and received, there's no need to raise exception (exception creation is expensive)
| @@ -67,7 +67,7 @@ final class ClasspathSdkHttpServiceProvider<T> implements SdkHttpServiceProvider | |||
|
|
|||
| @Override | |||
| public Optional<T> loadService() { | |||
| Queue<T> impls = new PriorityQueue<>(Comparator.comparingInt(o -> httpServicesPriority.getOrDefault(o.getClass(), | |||
| Queue<T> impls = new PriorityQueue<>(Comparator.comparingInt(o -> httpServicesPriority.getOrDefault(o.getClass().getName(), | |||
There was a problem hiding this comment.
This fixes the bug I found during testing that the priority order was not honored. I updated tests accordingly to catch this issue
| @@ -201,87 +194,6 @@ private void executeRequest(CrtAsyncRequestContext executionContext, | |||
| } | |||
| } | |||
|
|
|||
| private static final class InputStreamAdaptingHttpStreamResponseHandler implements HttpStreamResponseHandler { | |||
There was a problem hiding this comment.
I moved this class to a separate class.
|
Nice, can you give a brief overview of the optimizations? |
zoewangg
force-pushed
the
zoewang/master/crt-performanceOptimization
branch
from
4b58129 to
0a95cea
Compare
yup, added it in the description |
|
* Synchronous CRT Http Client SPI with tests (#4405) * implemented synchronous crt java http client with tests. --------- Co-authored-by: Waqar Ahmed Khan <[email protected]> Co-authored-by: Dongie Agnir <[email protected]> * Various refactoring on CRT Sync HTTP Client (#4749) * Make sure CRT resources are closed (#4753) * Load HTTP implementations from classpath based on priority order (#4774) * Various performance optimization for CRT sync HTTP client (#4776) * Various performance optimization for CRT sync HTTP client * Return as soon as the stream starts if there is a payload in sync http client * Clean up tests and add changelog entry * Bump minor version to 2.22.0 * UrlConnectionSdkHttpService should have higher priority than AwsCrtSdkHttpService * Remove CrtResource.waitForNoResources() from integration tests because it increases test time and makes tests flaky especially when tests are running in parallel. We still verify it in wiremock tests and stability tests --------- Co-authored-by: Jonathan M. Henson <[email protected]> Co-authored-by: Waqar Ahmed Khan <[email protected]> Co-authored-by: Dongie Agnir <[email protected]>
Motivation and Context
Various performance optimization for CRT sync HTTP client:
InputStreamSubscriber: not throw exception whencloseis invoked if it has buffered all content to avoid exception creation.InputStreamAdaptingHttpStreamResponseHandler: remove volatile for simplePublisher variable since it's unncesssaryprivate volatile SimplePublisher<ByteBuffer> simplePublisher;InputStreamAdaptingHttpStreamResponseHandler: complete future as soon as streaming starts. Previously, the future completes after streaming has finished, which means we'll buffer all data in memory first before user starts to read from the inputstream