HDDS-12110. Optimize memory overhead for OM background tasks. #7743
Conversation
|
Thanks for the patch @devmadhuu. LGTM otherwise |
Thanks @SaketaChalamchala for the review. I have optimized code in |
There was a problem hiding this comment.
Thanks for this improvement @devmadhuu! Please take a look at my comments, overall my question is that shouldn't we initialise these in the constructor?
private Collection<String> tables;
private HashMap<String, Long> objectCountMap;
private HashMap<String, Long> unReplicatedSizeMap;
private HashMap<String, Long> replicatedSizeMap;
| // Initialize maps to store count and size information | ||
| HashMap<String, Long> objectCountMap = initializeCountMap(); | ||
| HashMap<String, Long> unReplicatedSizeMap = initializeSizeMap(false); | ||
| HashMap<String, Long> replicatedSizeMap = initializeSizeMap(true); | ||
| final Collection<String> taskTables = getTaskTables(); |
There was a problem hiding this comment.
Aren't the map initialisations and the getTaskTables() still necessary here?
There was a problem hiding this comment.
Aren't the map initialisations and the
getTaskTables()still necessary here?
Once tables and maps are initialised once in reprocess, they will be available in each delta OM sync. Do you have any other use case in mind ?
| } | ||
|
|
||
| /** | ||
| * Initializes and returns a count map with the counts for the tables. | ||
| * | ||
| * @return The count map containing the counts for each table. | ||
| */ | ||
| private HashMap<String, Long> initializeCountMap() { | ||
| Collection<String> tables = getTaskTables(); |
There was a problem hiding this comment.
How do we make sure that tables is initialised here?
There was a problem hiding this comment.
Before initialising count map , we are already initialising taskTables in reprocess. Please explain what issue you see here.
In restart or upgrade case it is ok to initialise them in constructor because recon OM DB will already be there and |
| Field tableField = OmTableInsightTask.class.getDeclaredField("tables"); | ||
| tableField.setAccessible(true); | ||
| tableField.set(omTableInsightTask, omTableInsightTask.getTaskTables()); |
There was a problem hiding this comment.
Would you like to add setXXX setter with onlyForTesting tag for them?
What changes were proposed in this pull request?
This PR change is to reduce creation of too many local hashmap and other objects on repeated processing of OM events during delta sync of Recon with OM.
Some OM background tasks need code optimisations where the process method is creating static maps and other objects again and again with every run which are not needed, so that memory and GC overhead can be reduced.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-12110
How was this patch tested?
Patch is tested using existing junit tests.