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

Heap pressure and OutOfMemory(OOM) when looking up large number of config keys #8767

Closed
logovind opened this issue May 16, 2024 · 9 comments
Closed
Assignees
Labels
3.x Issues for 3.x version branch 4.x Version 4.x config
Projects
Milestone

Comments

@logovind
Copy link

Environment Details

  • Helidon Version: 4.0.8
  • Helidon SE or Helidon MP: Helidon MP
  • JDK version: JDK 21.0.3
  • OS: Oracle Linux
  • Docker version (if applicable):

Problem Description

we tested Helidon Config with Helidon 4(Version 4.0.8) using sample app. During the test, we found in one of the scenario the sample app continuously look for some config entries. It results in heap growth leading to OOM. checking further we saw the growth is primarily coming from "io.helidon.config.mp.MpEnvironmentVariablesSource". The implementation of this has the cache with concurrent hashmap and all the looked up config items getting added to cache even when environment variable corresponding to the key does not exists.

@github-actions github-actions bot added this to Triage in Backlog May 16, 2024
@logovind
Copy link
Author

Helidon code snippet MpEnvironmentVariablesSource.getValue()

https://github.com/helidon-io/helidon/blob/main/config/config-mp/src/main/java/io/helidon/config/mp/MpEnvironmentVariablesSource.java


public String getValue(String propertyName) {
    // environment variable config source is immutable - we can safely cache all requested keys, so we
    // do not execute the regular expression on every get
    return cache.computeIfAbsent(propertyName, theKey -> {
        // According to the spec, we have three ways of looking for a property
        // 1. Exact match
        String result = env.get(propertyName);
        if (null != result) {
            return new Cached(result);
        }
        // 2. replace non alphanumeric characters with _
        String rule2 = rule2(propertyName);
        result = env.get(rule2);
        if (null != result) {
            return new Cached(result);
        }
        // 3. replace same as above, but uppercase
        String rule3 = rule2.toUpperCase();
        result = env.get(rule3);
        return new Cached(result);
    }).value;
}

@logovind
Copy link
Author

To simulate this issue we used following code in the sample app where it uses randomly gerated string as the key.

test("Get_Config_with_default_value_test", responseMap, () -> StringUtils.equalsAnyIgnoreCase(config.getString(UUID.randomUUID().toString(), RANDOM), RANDOM));
test("Optional_config_test", responseMap, () -> config.getOptional(UUID.randomUUID().toString(), String.class).isEmpty());

Heap footprint:

Class Name | Objects | Shallow Heap | Retained Heap
io.helidon.config.mp.MpConfigImpl | 1 | 32 | >= 63,09,50,784
io.helidon.config.mp.MpEnvironmentVariablesSource | 2 | 48 | >= 37,46,57,888
io.helidon.config.mp.MpEnvironmentVariablesSource$Cached| 64,07,188 | 10,25,15,008 | >= 10,25,15,008

private final Map<String, Cached> cache = new ConcurrentHashMap<>();

we see that from the heap dump the cache which is implemented as concurrent hashmap in io.helidon.config.mp.MpEnvironmentVariablesSource ended up with ~3M Hash entries with those random keys based on the run from the sample app.

some of sample entries below:

Key |Value
9d3a23c9-0f74-43b2-8d9a-8a35d41e3062. | io.helidon.config.mp.MpEnvironmentVariablesSource$Cached @ 0xed496ba8
261469de-2937-4eeb-9875-2a0a282e19a3 | io.helidon.config.mp.MpEnvironmentVariablesSource$Cached @ 0xeff02598
762d84c3-e763-4410-93f7-5eb2943280e9 | io.helidon.config.mp.MpEnvironmentVariablesSource$Cached @ 0xe87e5008
9f12c260-5a2b-465e-90e9-85e238063c36 | io.helidon.config.mp.MpEnvironmentVariablesSource$Cached @ 0xf47121c0

@logovind
Copy link
Author

we tried out fix with code MpEnvironmentVariablesSource.getValue() modified as follows where as the entries added to cache only if the values exists.

public String getValue(String propertyName) {
    // environment variable config source is immutable - we can safely cache all requested keys, so we
    // do not execute the regular expression on every get
     Cached retval = cache.computeIfAbsent(propertyName, theKey -> {
        // According to the spec, we have three ways of looking for a property
        // 1. Exact match
        String result = env.get(propertyName);
        if (null != result) {
            return new Cached(result);
        }
        // 2. replace non alphanumeric characters with _
        String rule2 = rule2(propertyName);
        result = env.get(rule2);
        if (null != result) {
            return new Cached(result);
        }
        // 3. replace same as above, but uppercase
        String rule3 = rule2.toUpperCase();
        result = env.get(rule3);
        if (null != result) {
           return new Cached(result);
        }
        return null;
    }); 
    if (retval != null) {
           return retval.value;
    } else {
      return null;
    }
}

@logovind
Copy link
Author

Below the code diff:

MpEnvironmentVariablesSource_code_diff.txt

@logovind
Copy link
Author

we re-ran the same test with above code changes.

with this, we no longer see the heap pressure and OOM. non-existing Looked up items are no longer added to the cache.

Heap footprint after the code changes:

Class Name | Objects | Shallow Heap | Retained Heap

io.helidon.config.mp.MpEnvironmentVariablesSource$Cached| 164 | 2,624 | >= 2,624
io.helidon.config.mp.MpEnvironmentVariablesSource | 2 | 48 | >= 7,712
io.helidon.config.mp.MpConfigImpl | 1 | 32 | >= 31,856

@spericas spericas self-assigned this May 16, 2024
@spericas spericas added config 3.x Issues for 3.x version branch 4.x Version 4.x labels May 16, 2024
@spericas spericas added this to the 4.x milestone May 16, 2024
@spericas
Copy link
Member

@logovind I understand the issue and the proposed fix, but what is the use case that led you to this OOM? Seems a bit strange to look for all those undefined env variables using config. Could you describe it a bit more?

@vasanth-bhat
Copy link

vasanth-bhat commented May 16, 2024

Yes, it's bit of negative scenario and not a very common usage pattern
The test app used originally designed for functional coverage , has such use cases for random to check the functional behavior. The same app was used for performance tests.

However the code path will be executed , for all config parameters , which may have been defined in other sources such config yaml's . For each the env source is checked before other sources, and since those do not exist as env variables ,all those config get added to the cache with null values.

@spericas
Copy link
Member

Issue has been resolved by establishing an upper bound on the cache size and an LRU algorithm. The alternative of not caching certain null values had other problems. See #8768.

Backlog automation moved this from In Progress to Closed May 20, 2024
@vasanth-bhat
Copy link

The official fix was tested with 4.1.0 , and. confirmed that bounding the cache avoids the OOM for this scenario,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Issues for 3.x version branch 4.x Version 4.x config
Projects
Backlog
  
Closed
Development

No branches or pull requests

4 participants