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

[Performance] Performance of main branch is worse compared to 2.x #9866

Closed
bharath-techie opened this issue Sep 7, 2023 · 6 comments · Fixed by #9993
Closed

[Performance] Performance of main branch is worse compared to 2.x #9866

bharath-techie opened this issue Sep 7, 2023 · 6 comments · Fixed by #9993
Labels
bug Something isn't working v3.0.0 Issues and PRs related to version 3.0.0

Comments

@bharath-techie
Copy link
Contributor

Describe the bug
We started experiencing OOM in asynchronous search repository 'main' branch for 'Submit asynchronous search' concurrent tests which spawns 50 threads and performs 'Submit asynchronous search' API call.

The tests run in 512 mb of heap.

Test details:
Rest test case which calls submit async search API.
"org.opensearch.search.asynchronous.restIT.AsynchronousSearchSettingsIT.testMaxRunningAsynchronousSearchContexts"

In OpenSearch repository , I created a similar rest test which runs for Search API 50 times concurrently. And also changed the test heap size to 512 mb , to make it par with async search and any tests' default settings. ( opensearch tests runs in 3G heap)

In 2.x , even with 300 MB of max heap I'm able to run the test.
But in main branch the test fails with OOM even with 512 mb of max heap.
I need to allocate around 1GB of heap to run the same test in 'main' branch.

So performance of 'main' branch of OpenSearch seems to be worse than '2.x'.

To Reproduce
Steps to reproduce the behavior:

  1. https://github.com/bharath-techie/OpenSearch/pull/15/files - I have the changes in this PR to run search 50 times in 512 mb of heap.
  2. Run in 2.x - it passes
  3. Run in main - it fails
  4. Keep increasing the heap in main to find the limit at which the test passes
  5. Keep decreasing the heap in 2.x to find the limit at which the test fails

"./gradlew :client:rest-high-level:integTest --tests "org.opensearch.client.PitIT.testMaxRunningSearches -Dtest.heaps.size=1G"

Expected behavior
The 'main' branch and '2.x' branch should have similar performance.

Plugins
Rest high level client tests - so no plugins are enabled

Screenshots
If applicable, add screenshots to help explain your problem.

Host/Environment (please complete the following information):

  • OS: [e.g. iOS]
  • Version [e.g. 22]

Additional context
Add any other context about the problem here.

@bharath-techie bharath-techie added bug Something isn't working untriaged labels Sep 7, 2023
@reta
Copy link
Collaborator

reta commented Sep 7, 2023

@bharath-techie thanks a lot for raising that, I saw you've added -XX:+HeapDumpOnOutOfMemoryError, do you happens to have the heap dump(s) already you could share? One of the core diffs between main vs 2.x is Apache Lucene 9.8 vs 9.7, but that is just a guess. Thank you.

@bharath-techie
Copy link
Contributor Author

heap dump.zip

Hi @reta , uploading couple of heap dumps ^

@reta
Copy link
Collaborator

reta commented Sep 11, 2023

This is interesting, both dumps show the buffers kept by Apache HttpClient 5 (HeapBufferedAsyncEntityConsumer) however the resources (the byte buffers) are supposed to be released.

image

@msfroh
Copy link
Collaborator

msfroh commented Sep 11, 2023

@reta -- I'm wondering if it's because HeapBufferedAsyncEntityConsumer allocates the full 100MB array every time, instead of treating that as an upper-bound limit:

I think that buffer should be allocated as e.g. 1kB or 10kB be allowed to grow to 100MB.

@reta
Copy link
Collaborator

reta commented Sep 11, 2023

@msfroh yeah, I think its eager behavior is harmful, we could basically:

  • set smaller buffers (providing HttpAsyncResponseConsumerFactory)
  • or allocate by demand (but that would require copying sadly)

Will take a closer look tomorrow, thanks @msfroh !

@msfroh
Copy link
Collaborator

msfroh commented Sep 11, 2023

The good news is that ByteBuffer already takes care of reallocating on demand in the append methods.

Its expand behavior either jumps straight to the target size or doubles the size of the buffer (whichever is bigger):

    private void expand(final int newlen) {
        final byte[] newArray = new byte[Math.max(this.array.length << 1, newlen)];
        System.arraycopy(this.array, 0, newArray, 0, this.len);
        this.array = newArray;
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working v3.0.0 Issues and PRs related to version 3.0.0
Projects
None yet
3 participants