-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Implement byte array reusage in nio transport #27051
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
Changes from all commits
b76039b
5c4f9c2
198cd51
6294630
9496186
358983e
0fdf0be
f447893
64ce813
f197df7
636d504
d28cc9d
fadaf73
4d7dd1b
962e493
7844f5e
4c542cc
fb6c81e
95f2ff1
498e8e0
caea6d9
1168d9b
1161c83
af21323
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| /* | ||
| * Licensed to Elasticsearch under one or more contributor | ||
| * license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright | ||
| * ownership. Elasticsearch licenses this file to you under | ||
| * the Apache License, Version 2.0 (the "License"); you may | ||
| * not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package org.elasticsearch.common.collect; | ||
|
|
||
| import com.carrotsearch.hppc.BufferAllocationException; | ||
| import com.carrotsearch.hppc.ObjectArrayDeque; | ||
|
|
||
| /** | ||
| * This is an {@link ObjectArrayDeque} that allows indexed O(1) access to its contents. | ||
| * Additionally, it overrides the catching on {@link OutOfMemoryError} on resize. | ||
| * | ||
| * @param <KType> the types contained in the deque | ||
| */ | ||
| public class IndexedArrayDeque<KType> extends ObjectArrayDeque<KType> { | ||
|
|
||
| public IndexedArrayDeque(int initialSize) { | ||
| super(initialSize); | ||
| } | ||
|
|
||
| public KType get(int i) { | ||
| Object[] rawBuffer = buffer; | ||
| int actualIndex = (head + i) % rawBuffer.length; | ||
| @SuppressWarnings("unchecked") | ||
| KType value = (KType) rawBuffer[actualIndex]; | ||
| return value; | ||
| } | ||
|
|
||
| @Override | ||
| protected void ensureBufferSpace(int expectedAdditions) { | ||
| try { | ||
| super.ensureBufferSpace(expectedAdditions); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh man that is terrible. We should contribute this back to hppc. Can you make sure we do this? I don't think it should ever catch OOM
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh and, good catch! |
||
| } catch (BufferAllocationException e) { | ||
| Throwable cause = e.getCause(); | ||
| if (cause instanceof OutOfMemoryError) { | ||
| throw (OutOfMemoryError) cause; | ||
| } | ||
| throw e; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -287,7 +287,7 @@ protected boolean enableWarningsCheck() { | |
|
|
||
| @After | ||
| public final void after() throws Exception { | ||
| checkStaticState(); | ||
| checkStaticState(false); | ||
| // We check threadContext != null rather than enableWarningsCheck() | ||
| // because after methods are still called in the event that before | ||
| // methods failed, in which case threadContext might not have been | ||
|
|
@@ -393,8 +393,10 @@ public void log(StatusData data) { | |
| } | ||
|
|
||
| // separate method so that this can be checked again after suite scoped cluster is shut down | ||
| protected static void checkStaticState() throws Exception { | ||
| MockPageCacheRecycler.ensureAllPagesAreReleased(); | ||
| protected static void checkStaticState(boolean afterClass) throws Exception { | ||
| if (afterClass) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need this in general or can we maybe only do this in specific test cases?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I view this as a long term fix. I just wanted to full CI run without failures caused by holding bytes in between tests. We maybe should talk about the best way to deal with this.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok understood. maybe mark it with |
||
| MockPageCacheRecycler.ensureAllPagesAreReleased(); | ||
| } | ||
| MockBigArrays.ensureAllArraysAreReleased(); | ||
|
|
||
| // ensure no one changed the status logger level on us | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the closeable contract says it's closeable multple times so we are good I guess.