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

Allow to override the 1:1 ratio between Xmx & off-heap amounts #365

Merged
merged 1 commit into from
Nov 29, 2019
Merged

Allow to override the 1:1 ratio between Xmx & off-heap amounts #365

merged 1 commit into from
Nov 29, 2019

Conversation

maxime-michel
Copy link
Contributor

@saudet
Copy link
Member

saudet commented Nov 26, 2019

I thought we could apply this to Runtime.getRuntime().maxMemory(), wouldn't that make more sense?

@maxime-michel
Copy link
Contributor Author

Simplifying like that you mean? It does make more sense.

@saudet
Copy link
Member

saudet commented Nov 26, 2019

And let's do the same for the second Runtime.getRuntime().maxMemory() too.

@maxime-michel
Copy link
Contributor Author

Sure. I put that operation in a variable named rm in order to avoid a nested ternary operator at L485. Let me know if that works for you. Thanks!

@maxime-michel
Copy link
Contributor Author

What about documentation, should we update the page on memory management by adding the following under Configuring Memory Limit?

-Dorg.bytedeco.javacpp.maxMemoryRatio - this allows to override the ratio by which to multiply Xmx in order to define maxBytes as well as maxPhysicalBytes, in case maxBytes and maxPhysicalBytes aren't defined. By default, 1 Xmx => 1 maxBytes and 1 Xmx => 2 maxPhysicalBytes.

@saudet
Copy link
Member

saudet commented Nov 27, 2019

I'm sorry, this doesn't actually work well. If we set maxMemoryRatio to something like 0.1, then maxPhysicalBytes gets 20% of maxMemory, which is likely to be hit quite often since we've given 5 times more than that to the JVM. I think we could have only 1 value for maxBytes for now and give it an appropriate name like maxBytesScale. What do you think?

Yes, we should also update DL4J's documentation there once we're done: https://github.com/eclipse/deeplearning4j/blob/master/docs/deeplearning4j/templates/config-memory.md

@maxime-michel
Copy link
Contributor Author

Totally. Sorry about that, I'll get it fixed ASAP and submit a separate PR for documentation.

@maxime-michel
Copy link
Contributor Author

OK, I've upgraded the PR so that:

  • a 4GB instance
  • with 2GB of Xmx
  • and a maxBytesScale of 0.1

Would result in:

  • maxBytes = 2G * 0.1 = 200MB
  • maxPhysicalBytes = 2G + 200MB = 2.2G

@saudet
Copy link
Member

saudet commented Nov 27, 2019

I still feel this is more complexity than users should be dealing with though. Is a value like "0.1" parsed into double or something more precise? Is the scale also applied when specifying maxBytes, or not?

I think I've just had an idea about a much cleaner way to deal with that. What about adding a new "unit" like %, in addition to the usual ones like M and G? Something like maxBytes at 10% would mean 10 * Runtime.maxMemory / 100, and it would work just as well for maxPhysicalBytes with values like 150%. Very easy to read too. What do you think?

@maxime-michel
Copy link
Contributor Author

I like that better indeed. Something like this?

diff --git a/src/main/java/org/bytedeco/javacpp/Pointer.java b/src/main/java/org/bytedeco/javacpp/Pointer.java
index 5efcaf8..e98ab73 100644
--- a/src/main/java/org/bytedeco/javacpp/Pointer.java
+++ b/src/main/java/org/bytedeco/javacpp/Pointer.java
@@ -421,7 +421,7 @@ public class Pointer implements AutoCloseable {
         }
     }

-    public static long parseBytes(String string) throws NumberFormatException {
+    public static long parseBytes(String string, long maxMemory) throws NumberFormatException {
         int i = 0;
         while (i < string.length()) {
             if (!Character.isDigit(string.charAt(i))) {
@@ -435,6 +435,7 @@ public class Pointer implements AutoCloseable {
             case "g": case "gb": size *= 1024L; /* no break */
             case "m": case "mb": size *= 1024L; /* no break */
             case "k": case "kb": size *= 1024L; /* no break */
+            case "%": size = size * maxMemory * 1024L / 100;
             case "": break;
             default: throw new NumberFormatException("Cannot parse into bytes: " + string);
         }
@@ -457,7 +458,7 @@ public class Pointer implements AutoCloseable {
         s = System.getProperty("org.bytedeco.javacpp.maxBytes", s);
         if (s != null && s.length() > 0) {
             try {
-                m = parseBytes(s);
+                m = parseBytes(s, m);
             } catch (NumberFormatException e) {
                 throw new RuntimeException(e);
             }

@saudet
Copy link
Member

saudet commented Nov 28, 2019

Yes, that's how I see it. Inside that method maxMemory is more like a relativeUnit than a "max" though.

@maxime-michel
Copy link
Contributor Author

I've updated the code accordingly and pushed it. relativeUnit doesn't sound to me like a name for a variable that would hold Runtime.getRuntime().maxMemory() and then maxBytes + Runtime.getRuntime().maxMemory(), but I have no better idea so I don't mind.

I'm more concerned about tests, though. Even on master they are mostly failing on my machine. And even if they weren't, they look like black magic to me. Would you mind giving me a hand there?

@saudet
Copy link
Member

saudet commented Nov 28, 2019

Ah, that was it see_no_evilBy the way, did you know you can cache the Maven repository to speed up Travis builds?

cache:
  directories:
  - "$HOME/repository"

Yes, I know, it just didn't get added here like elsewhere for some reason:
https://github.com/bytedeco/javacv/blob/master/.travis.yml

@saudet saudet merged commit cd09b71 into bytedeco:master Nov 29, 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.

None yet

2 participants