Skip to content

DB-2993: export used direct memory#14

Merged
stef1927 merged 3 commits intoriptano:dse-netty-4.1.13.Finalfrom
stef1927:dse-netty-4.1.13.Final
Mar 12, 2019
Merged

DB-2993: export used direct memory#14
stef1927 merged 3 commits intoriptano:dse-netty-4.1.13.Finalfrom
stef1927:dse-netty-4.1.13.Final

Conversation

@stef1927
Copy link
Copy Markdown

@stef1927 stef1927 commented Mar 6, 2019

I redirected the JVM NIO direct buffer allocations to PlatformDependent so long as these buffers were cleaned via PlatformDependent.

I've also changed PlatformDependent to measure allocated memory even when using the JVM NIO direct allocations. I then exported the memory allocated with PlatformDependent.usedDirectMemory(). The idea is to read this value in our own native memory metrics.

@sbtourist or @tjake would you be able to review?

cc @snazy

@tjake
Copy link
Copy Markdown

tjake commented Mar 6, 2019

Any reason not to do this on main netty repo?

@stef1927
Copy link
Copy Markdown
Author

stef1927 commented Mar 7, 2019

Any reason not to do this on main netty repo?

The patch adds JVM allocations to DIRECT_MEMORY_COUNTER, therefore changing its semantic functionality. I'm not so sure they'll accept this change in 4.1. Also, I wanted to merge DB-2993 relatively quickly. We can merge it without the metric if needed, I just thought it would be useful information to have.

@stef1927 stef1927 requested review from sbtourist and tjake March 11, 2019 02:48
Copy link
Copy Markdown

@sbtourist sbtourist left a comment

Choose a reason for hiding this comment

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

LGTM, just left a couple minor comments.

PlatformDependent0.setMemory(address, bytes, value);
}

public static ByteBuffer allocateDirect(int capacity) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why the "allocate" method has no Buffer suffix and vice versa for "free"?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

freeDirectBuffer() was an existing method, hence the confusion. Renamed, thanks.

}

/**
* Allocate a new {@link ByteBuffer} with the given {@code capacity}. {@link ByteBuffer}s allocated with
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm not sure why the JVM suffix versus calling it something like allocateDirectWithCleaner, but either ways this javadoc seems wrong as the method to free the buffer should be freeDirectBufferJVM right?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

noticed the same

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Renamed and fixed javadoc, thanks.

Copy link
Copy Markdown

@tjake tjake left a comment

Choose a reason for hiding this comment

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

I had the same comment about the javadoc

@stef1927
Copy link
Copy Markdown
Author

Thanks for the review. I've renamed the allocate and free methods and bumped the version.

@stef1927 stef1927 merged commit cefc52c into riptano:dse-netty-4.1.13.Final Mar 12, 2019
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