[
https://issues.apache.org/jira/browse/YARN-7136?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16150688#comment-16150688
]
Jason Lowe commented on YARN-7136:
----------------------------------
Thanks for updating the patch!
The findbugs exclusion entry should just change the filename for BaseResource
to Resource rather than add a new entry, since BaseResource no longer has the
method.
ResourcePBImpl's getResources method is redundant since it simply delegates to
the super method.
Resource's compareTo is now treating all the resources equally which is nice,
but that means for memory and vcores it will be doing string compares on the
names, hash set lookups on the units, and string comparisons on the units
before it ever gets to simply comparing the numbers which is all we really care
about for these resources. As I've mentioned in offline conversation, I don't
understand why we would ever want to do any unit comparisons in the scheduler
allocation code. It may make sense from a performance standpoint to have
something like a NormalizedResource that has pre-normalized all the
ResourceInformation entries to a base precision unit for that resource so the
scheduler can avoid doing any resource name comparisons (the resource index
match should be an equivalent test) and any unit conversions. That would avoid
all string comparisons and hash set lookups for resource comparison in the
scheduler main-path code, reducing it to simply a loop over an array of numbers
we can directly compare without further computation. The
ApplicationMasterService can marshal the app's requests into the normalized
resources before feeding the requests to the scheduler so the scheduler doesn't
have to ever worry about units. Worth a followup JIRA?
Similar performance hit for the Resources equals method since it now needs to
check names and units before checking values on memory and vcores which it did
not do before.
For the toString change this will start to show units for memory ("Mi") which
was not done before. Does memory need to have no units for
backwards-compatibility as it is for vcores? Otherwise it seems like as soon
as a custom resources is added memory will suddenly go from no units to "Mi"
units in toString as we switch from BaseResource.
In many places in DominantResourceCalculator:
{code}
for (int i = 0; i < ResourceUtils.getNumberOfKnownResourceTypes(); i++) {
{code}
getNumberOfKnownResourceTypes has to touch a volatile variable each time, so we
should cache this in a local rather than hit it every time in the loop.
Speaking of hitting this volatile variable, what happens if this value suddenly
changes to be larger than the array of resources passed in? In other words, it
seems broken that we're looking at a global variable to determine how many
entries in the array argument to look at as they could be inconsistent.
Thinking that if an admin suddenly refreshes the resource types to add one the
code could end up indexing off the end of an array argument that was just
passed to it before the refresh occurred.
DominantResourceCalculator#calculateSharesForMandatoryResources says it is
custom-built to only check two resources and assumes clusterRes only has two
entries yet is calling ResourceUtils.getNumberOfKnownResourceTypes() and
iterating that many times through clusterRes?
Same hitting-a-volatile-in-the-loop issue for Resources.
> 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, YARN-7136.YARN-3926.003.patch,
> YARN-7136.YARN-3926.004.patch, YARN-7136.YARN-3926.005.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
