Skip to content

Use netty maxDirectMemory instead of DirectMemoryUtils#2989

Merged
nicoloboschi merged 1 commit intoapache:masterfrom
hezhangjian:direct-memory
Apr 6, 2022
Merged

Use netty maxDirectMemory instead of DirectMemoryUtils#2989
nicoloboschi merged 1 commit intoapache:masterfrom
hezhangjian:direct-memory

Conversation

@hezhangjian
Copy link
Copy Markdown
Member

@hezhangjian hezhangjian commented Jan 13, 2022

Motivation

Our DirectMemoryUtils has huge limit, it can't work well with other jvm. The Netty PlatformDependent.maxDirectMemory(); is more generic.

Changes

Use PlatformDependent.maxDirectMemory(); instead of DirectMemoryUtils

@hezhangjian
Copy link
Copy Markdown
Member Author

hezhangjian commented Jan 17, 2022

@eolivelli @dlg99 @merlimat @pkumar-singh PTAL

dlg99
dlg99 requested changes Jan 19, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@dlg99 dlg99 left a comment

Choose a reason for hiding this comment

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

I don't think this is the right change. As I understand you have a situation when direct memory usage results in problems for other processes, possibly linux's OOM killer kicking in.

https://netty.io/4.1/xref/io/netty/util/internal/PlatformDependent.html#L153 describes how PlatformDependent.maxDirectMemory is controlled and used in netty. It doesn't limit direct memory for the JVM, -XX:MaxDirectMemorySize jvm parameter does.
The idea is to set -XX:MaxDirectMemorySize for the JVM and possibly limit (up to that max) for netty,

I suggest taking a look at the shell scripts for BK for examples of -XX:MaxDirectMemorySize usage and setting it to the appropriate value.
Please let me know if I misunderstood something.
.

@hezhangjian
Copy link
Copy Markdown
Member Author

hezhangjian commented Jan 19, 2022

@dlg99 Hi, We have a bookkeeper server, which is conifugred as "-Xmx6G -XDirectMemory4G". And we are using huawei jdk11. DirectMemoryUtils can't reed the sun.misc.VM. maxDirectMemory, fall back to the heap memory, which is 6G beyond the real -XDirectMemory.
Instead of enhace DirectMemoryUtils, we can use PlatformDependent.maxDirectMemory()

@dlg99
Copy link
Copy Markdown
Contributor

dlg99 commented Jan 20, 2022

Just switching to PlatformDependent.maxDirectMemory() feels like a misuse of it plus it might not work for some people if they e.g. set io.netty.maxDirectMemory greater than zero.
I'd try something like a configurable strategy for the DirectMemoryUtils to specify how to read the max memory (via Oracle JD specific methods, via Huawei's, via config parameter, etc)

@hezhangjian
Copy link
Copy Markdown
Member Author

hezhangjian commented Feb 13, 2022

@dlg99 You are right. set io.netty.maxDirectMemory greater than zero might not work.
So we can enhance the DirectMemoryUtils with netty's code(They are Apache LICENSE too)
PTAL, thanks

@hezhangjian
Copy link
Copy Markdown
Member Author

hezhangjian commented Feb 15, 2022

rerun failure checks

eolivelli
eolivelli requested changes Feb 15, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Did you copy paste the code from Netty?

@hezhangjian
Copy link
Copy Markdown
Member Author

hezhangjian commented Feb 15, 2022

Did you copy paste the code from Netty?

Yes, I did it because that these are more genernal, and netty is under apache license.

@eolivelli
Copy link
Copy Markdown
Contributor

eolivelli commented Feb 15, 2022

What about sending a PR to Netty in order to expose such value?
Netty project is very open to contributions, I sent some PRs in the past.
They also have a very frequent release cycle.

I would prefer to not copy paste, because in Netty they are maintaining that code and adapt it to new Java versions, and we will keep this version forever

@hezhangjian
Copy link
Copy Markdown
Member Author

hezhangjian commented Feb 16, 2022

@eolivelli Thanks for your advice, I will try

@hezhangjian hezhangjian marked this pull request as draft February 16, 2022 00:11
@hezhangjian hezhangjian mentioned this pull request Feb 16, 2022
Closed
@hezhangjian
Copy link
Copy Markdown
Member Author

hezhangjian commented Feb 23, 2022

@eolivelli @dlg99 netty/netty#12118 is merged. I will reopen this PR when we update to netty next version.

@hezhangjian hezhangjian mentioned this pull request Mar 11, 2022
Closed
@hezhangjian hezhangjian mentioned this pull request Mar 31, 2022
Merged
@hezhangjian hezhangjian marked this pull request as ready for review April 3, 2022 08:39
@hezhangjian
Copy link
Copy Markdown
Member Author

hezhangjian commented Apr 3, 2022

@eolivelli @dlg99 @merlimat @pkumar-singh PTAL again. Netty has exposed a util method for user to get maxDirectoryMemory. See netty/netty#12118

eolivelli
eolivelli approved these changes Apr 3, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Great!

@hezhangjian
Copy link
Copy Markdown
Member Author

hezhangjian commented Apr 5, 2022

@eolivelli @dlg99 @merlimat @pkumar-singh @nicoloboschi
Please help merge this PR when you have time, thanks :)

@nicoloboschi nicoloboschi added this to the 4.16.0 milestone Apr 6, 2022
@nicoloboschi nicoloboschi merged commit d4eba62 into apache:master Apr 6, 2022
@hezhangjian hezhangjian deleted the direct-memory branch April 6, 2022 08:34
This was referenced Apr 20, 2022
Closed
Merged
hezhangjian pushed a commit to apache/pulsar that referenced this pull request May 15, 2022
use netty PlatformDependent.estimateMaxDirectMemory (#15238)
9d5d110 
### Motivation
- `PlatformDependent.maxDirectMemory()` can be inaccurate if `io.netty.maxDirectMemory` are setted
- Bookkeeper's `DirectMemoryUtils` is not worked within some jdk releases.
In netty 4.1.75, they introduced a new method `PlatformDependent.estimateMaxDirectMemory` to help users get maxDirectMemory. Since netty's this method works well on many jdk releases, use this to replace below two.
PS: `DirectMemoryUtils` has been removed from bookkeeper in apache/bookkeeper#2989
### Modifications
- use `PlatformDependent.estimateMaxDirectMemory` instead
@dlg99 dlg99 mentioned this pull request Jun 28, 2022
Merged
@hangc0276 hangc0276 mentioned this pull request Jun 29, 2022
Merged
hezhangjian pushed a commit that referenced this pull request Jun 30, 2022
@hangc0276
use estimateMaxDirectMemory instead of maxDirectMemory (#3375)
649d2bb 
### Motivation
#2989

### Modification
Use estimateMaxDirectMemory instead of maxDirectMemory
@hangc0276 hangc0276 mentioned this pull request Jul 31, 2022
Closed
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
Use netty maxDirectMemory instead of DirectMemoryUtils
744366f 
### Motivation

Our `DirectMemoryUtils` has huge limit, it can't work well with other jvm. The Netty `PlatformDependent.maxDirectMemory();` is more generic.

### Changes
Use `PlatformDependent.maxDirectMemory();` instead of `DirectMemoryUtils`

Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Andrey Yegorov <None>, Matteo Merli <mmerli@apache.org>, Nicolò Boschi <boschi1997@gmail.com>

This closes apache#2989 from Shoothzj/direct-memory
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
@hangc0276
use estimateMaxDirectMemory instead of maxDirectMemory (apache#3375)
1f6475b 
### Motivation
apache#2989

### Modification
Use estimateMaxDirectMemory instead of maxDirectMemory
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@merlimat merlimat merlimat approved these changes

@dlg99 dlg99 dlg99 approved these changes

@eolivelli eolivelli eolivelli approved these changes

@nicoloboschi nicoloboschi nicoloboschi approved these changes

Assignees

@hezhangjian hezhangjian

Labels

area/bookie release/4.16.0 type/improvement

Projects

None yet

Milestone

4.16.0

Development

Successfully merging this pull request may close these issues.

5 participants

@hezhangjian @dlg99 @eolivelli @merlimat @nicoloboschi