[
https://issues.apache.org/jira/browse/YARN-7136?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16149144#comment-16149144
]
Jason Lowe commented on YARN-7136:
----------------------------------
Thanks for the patch!
In the Resource equals method, is there a need to start doing the compare from
index 0? Seems like we've already covered indices 0 and 1 before we get to the
loop.
Similarly in the compare method, why does the code do this instead of just
starting the loop at 2 instead of 0?
{code}
for (int i = 0; i < maxLength; i++) {
// For memory and vcores, we can skip the loop as it's already
// compared.
if (i < 2) {
continue;
}
{code}
The code should not be runtime-checking to skip memory and vcores in the
hashCode or toString methods. We should either treat memory and vcores just
like the others, or we should start iterating at index 2 rather than do string
compares within the loop. We have an array, we know what the first two entries
always are, let's leverage that rather than rediscover it on every method call
for the first two iterations of every loop.
Speaking of the compareTo method, wondering if there's a bug unrelated to this.
The code is checking if the entry names are the same before calling
entry.compareTo(otherEntry). If the names are _not_ the same, it doesn't break
early and return they're different. That seems wrong. Seems to me it should
blindly call entry.compareTo(otherEntry) whether the names match or not. Are
resource names allowed to be at different indices for two different resources?
I don't think it's legal for the DominantResourceCalculator to cache
ResourceUtils.getNumberOfKnownResourceTypes as a static. That means it will
cache that value upon class load, but that value isn't a constant. It can
change at runtime which is why it's a volatile rather than a final in
ResourceUtils. In general I don't see how it's going to be valid to cache a
volatile variable in a class static unless the volatile really shouldn't be
volatile in the first place, and if it isn't then we probably should mark it as
final in its original place and not need to cache it here.
> Additional Performance Improvement for Resource Profile Feature
> ---------------------------------------------------------------
>
> Key: YARN-7136
> URL: https://issues.apache.org/jira/browse/YARN-7136
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: nodemanager, resourcemanager
> Reporter: Wangda Tan
> Assignee: Wangda Tan
> Priority: Critical
> Attachments: YARN-7136.001.patch, YARN-7136.YARN-3926.001.patch,
> YARN-7136.YARN-3926.002.patch
>
>
> This JIRA is plan to add following misc perf improvements:
> 1) Use final int in Resources/ResourceCalculator to cache
> #known-resource-types. (Significant improvement).
> 2) Catch Java's ArrayOutOfBound Exception instead of checking array.length
> every time. (Significant improvement).
> 3) Avoid setUnit validation (which is a HashSet lookup) when initialize
> default Memory/VCores ResourceInformation (Significant improvement).
> 4) Avoid unnecessary loop array in Resource#toString/hashCode. (Some
> improvement).
> 5) Removed readOnlyResources in BaseResource. (Minor improvement).
> 6) Removed enum: MandatoryResources, use final integer instead. (Minor
> improvement).
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
