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

Use Runtime.getRuntime().totalMemory() to calculate 1% of the heap memory #204

Closed
zysaaa opened this issue Nov 8, 2021 · 3 comments · Fixed by #264
Closed

Use Runtime.getRuntime().totalMemory() to calculate 1% of the heap memory #204

zysaaa opened this issue Nov 8, 2021 · 3 comments · Fixed by #264
Labels

Comments

@zysaaa
Copy link

zysaaa commented Nov 8, 2021

By default, 1% of memory is used as the queue memory limit in AsyncReporter :

   int queuedMaxBytes = onePercentOfMemory();

    static int onePercentOfMemory() {
      long result = (long) (Runtime.getRuntime().totalMemory() * 0.01);
      // don't overflow in the rare case 1% of memory is larger than 2 GiB!
      return (int) Math.max(Math.min(Integer.MAX_VALUE, result), Integer.MIN_VALUE);
    }

It uses Runtime.getRuntime().totalMemory() which is that the JVM has allocated thus far. This isn't necessarily what is in use or the maximum.
image

Is this calling the wrong API or is it designed that way? From what I understand, it seems like maxMemory should be used here? With totalMemory, the byte limit is different each time the app is started.

@zysaaa zysaaa added the bug label Nov 8, 2021
@shakuzen
Copy link
Member

shakuzen commented Nov 8, 2021

The commit that added it goes back to fd6ab10. Off the top of my head, I suppose 1% of the max heap probably makes more sense than the somewhat arbitrary measure of currently allocated heap. It's interesting this hasn't come up before. Did you run into an issue where your reporter queue was limited by the queued max bytes? I suppose this isn't an issue in the case -Xms and -Xmx are the same value.

@zysaaa
Copy link
Author

zysaaa commented Nov 8, 2021

I didn't run into an issue where my queue was limited by the queued max bytes. Actually, I am trying to understand the role of using ByteBoundedQueue. And I found this totalMemory() while viewing the source code. I think using maxMemory makes more sense, as specified in readme file:

Property Description
queuedMaxBytes Maximum backlog of span bytes reported vs sent. Corresponds to ReporterMetrics.updateQueuedBytes. Default 1% of heap

1% of the heap memory mentioned here should refer to the heap memory set by -Xmx instead of memory that JVM has allocated thus far which I think is uncertain and usually is a relatively small value when application start.

So actually I have another question about ByteBoundedQueue, according to my understanding, the purpose of using this queue is to avoid the accumulation of spans occupying too much memory. Suppose that after the application is started, 1% of the heap memory is allocated to the queue (default behavior). Assuming it is 100M, when the queue is full, the memory actually occupied by this queue is far more than 100M. Due to this change: #84, It seems that it was originally encoded first, and then put into the queue. In this way, because the queue is already a byte array after encoding, the actual memory occupied after the queue is full should be similar to the memory allocated to the queue. But after this change, it will call sizeInBytes first to calculate the number of bytes(Will eventually use String.length() in ZipkinV2JsonWriter#sizeInBytes), for example, in Java8, suppose there is a String "ABC" that takes up at least "ABC ". length() * 2 bytes in memory (a char need 2 bytes). This seems to cause the actual memory occupied after the queue is full to be quite different from what we set. @shakuzen

@codefromthecrypt
Copy link
Member

This default value was made for a simple default, and the sizing is imprecise for sure. I don't expect we'll want the complexity to try for precise sizing, neither try to be more heuristic about max size and assume users want to pre-allocate a larger heap for span reporting reasons alone (e.g. to make sure we use max value).

The best way out would be to

  • clarify the comment on the default value (to say it is about the current heap)
  • possibly add some language to say that the sizing is nuanced and about the size of the data and not including the overhead of the data.

I don't expect there to be value in doing more than this until there are more active maintainers, who are able to maintain the project and also maintain a more sophisticated sizing engine.

codefromthecrypt pushed a commit that referenced this issue Apr 12, 2024
Detailed rationale in RATIONALE.md

Closes #204
Closes openzipkin/brave#1426

Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt added a commit that referenced this issue Apr 12, 2024
Detailed rationale in RATIONALE.md

Closes #204
Closes openzipkin/brave#1426

Signed-off-by: Adrian Cole <adrian@tetrate.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants