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

Exclude attributes from Endpoint.toString() #6061

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ikhoon
Copy link
Contributor

@ikhoon ikhoon commented Jan 9, 2025

Motivation:

We observed OutOfMemoryError in internal CI tests when a new Endpoint is created.

com.linecorp.armeria.xds.client.endpoint.EndpointsPool$$Lambda$754/1708334230@3a58f44e
    java.lang.OutOfMemoryError: Java heap space
        at java.util.Arrays.copyOfRange(Arrays.java:3664)
        at java.lang.String.<init>(String.java:207)
        at java.lang.StringBuilder.toString(StringBuilder.java:412)
        at com.google.protobuf.TextFormat$Printer.printToString(TextFormat.java:593)
        at com.google.protobuf.AbstractMessage.toString(AbstractMessage.java:87)
        at java.lang.String.valueOf(String.java:2994)
        at java.lang.StringBuilder.append(StringBuilder.java:136)
        at com.linecorp.armeria.internal.shaded.guava.collect.AbstractMapEntry.toString(AbstractMapEntry.java:70)
        at java.lang.String.valueOf(String.java:2994)
        at java.lang.StringBuilder.append(StringBuilder.java:136)
        at com.linecorp.armeria.internal.shaded.guava.collect.Iterators.toString(Iterators.java:294)
        at com.linecorp.armeria.common.ImmutableAttributes.toString(ImmutableAttributes.java:183)
        at java.lang.String.valueOf(String.java:2994)
        at java.lang.StringBuilder.append(StringBuilder.java:136)
        at com.linecorp.armeria.client.Endpoint.generateToString(Endpoint.java:304)
        at com.linecorp.armeria.client.Endpoint.<init>(Endpoint.java:268)
        at com.linecorp.armeria.client.Endpoint.replaceAttrs(Endpoint.java:747)
        at com.linecorp.armeria.client.Endpoint.withAttr(Endpoint.java:707)
        at com.linecorp.armeria.xds.client.endpoint.EndpointsPool.withTimestamp(EndpointsPool.java:103)
        at com.linecorp.armeria.xds.client.endpoint.EndpointsPool.cacheAttributesAndDelegate(EndpointsPool.java:71)
        at com.linecorp.armeria.xds.client.endpoint.EndpointsPool.lambda$updateClusterSnapshot$1(EndpointsPool.java:62)
        at com.linecorp.armeria.xds.client.endpoint.EndpointsPool$$Lambda$754/1708334230.run(Unknown Source)

When an Endpoint is initicated, it pre-generates a string representation for caching and reuse. In our tests, it contained many attributes related to xDS for service discovery. It might be a good idea to remove attributes whose size is hard to predict from toString() and prevent unintended OOM.

In addition, the result size of toString() gets small, so I doubt that the pre-generated cache is useful for performance.

Modifications:

  • Remove attributes from toString().
  • Lazily build the string representation when toString() is called and don't cache the result.

Result:

Endpoint no longer includes attributes in .toString().

Motivation:

We observed `OutOfMemoryError` in internal CI tests when
a new `Endpoint` is created.
```
com.linecorp.armeria.xds.client.endpoint.EndpointsPool$$Lambda$754/1708334230@3a58f44e
    java.lang.OutOfMemoryError: Java heap space
        at java.util.Arrays.copyOfRange(Arrays.java:3664)
        at java.lang.String.<init>(String.java:207)
        at java.lang.StringBuilder.toString(StringBuilder.java:412)
        at com.google.protobuf.TextFormat$Printer.printToString(TextFormat.java:593)
        at com.google.protobuf.AbstractMessage.toString(AbstractMessage.java:87)
        at java.lang.String.valueOf(String.java:2994)
        at java.lang.StringBuilder.append(StringBuilder.java:136)
        at com.linecorp.armeria.internal.shaded.guava.collect.AbstractMapEntry.toString(AbstractMapEntry.java:70)
        at java.lang.String.valueOf(String.java:2994)
        at java.lang.StringBuilder.append(StringBuilder.java:136)
        at com.linecorp.armeria.internal.shaded.guava.collect.Iterators.toString(Iterators.java:294)
        at com.linecorp.armeria.common.ImmutableAttributes.toString(ImmutableAttributes.java:183)
        at java.lang.String.valueOf(String.java:2994)
        at java.lang.StringBuilder.append(StringBuilder.java:136)
        at com.linecorp.armeria.client.Endpoint.generateToString(Endpoint.java:304)
        at com.linecorp.armeria.client.Endpoint.<init>(Endpoint.java:268)
        at com.linecorp.armeria.client.Endpoint.replaceAttrs(Endpoint.java:747)
        at com.linecorp.armeria.client.Endpoint.withAttr(Endpoint.java:707)
        at com.linecorp.armeria.xds.client.endpoint.EndpointsPool.withTimestamp(EndpointsPool.java:103)
        at com.linecorp.armeria.xds.client.endpoint.EndpointsPool.cacheAttributesAndDelegate(EndpointsPool.java:71)
        at com.linecorp.armeria.xds.client.endpoint.EndpointsPool.lambda$updateClusterSnapshot$1(EndpointsPool.java:62)
        at com.linecorp.armeria.xds.client.endpoint.EndpointsPool$$Lambda$754/1708334230.run(Unknown Source)
```
When an `Endpoint` is initicated, it pre-generates a string
representation for caching and reuse. In our tests, it contained many
attributes related to xDS for service discovery.

It might be a good idea to remove attributes whose size is hard to
predict from `toString()` and prevent unintended OOM.

In addition, the result size of `toString()` gets small, I doubt that
the pre-generated cache is useful for performance.

Modifications:

- Remove attributes from `toString()`.
- Lazily build the string representation when `toString()` is called and
  don't cache the result.

Result:

`Endpoint` no longer include attributes in `.toString()`.
@ikhoon ikhoon added the defect label Jan 9, 2025
@ikhoon ikhoon added this to the 1.32.0 milestone Jan 9, 2025
Copy link
Contributor

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

👍

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 this pull request may close these issues.

3 participants