hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Daniel Templeton (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-6788) Improve performance of resource profile branch
Date Wed, 26 Jul 2017 22:03:00 GMT

    [ https://issues.apache.org/jira/browse/YARN-6788?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16102352#comment-16102352
] 

Daniel Templeton commented on YARN-6788:
----------------------------------------

Taking another pass at the review.  Here's my comments:

* {{Resource.getResourceValue()}} should just return {{getResourceInformation().getValue()}}
* {{Resource.setResourceInformation()}} and {{Resource.setResourceValue()}} should call {{getResourceInformation()}}
instead of duplicating the logic
* In {{Resource.equals()}} the _if_ can be simplified from: {code}      if (a == b || (a ==
null && b == null)) {
        continue;
      } else if ((a == null && b != null) || (a != null && b == null)) {
        return false;
      } else {
        if (a != null && !a.equals(b)) {
          return false;
        }
      }{code} into {code}      if ((a != b) && ((a == null) || !a.equals(b))) {
        return false;
      }{code}
* In {{Resource.compareTo()}}:{code}        for (ResourceInformation entry : thisResources)
{
          if (entry.getName().equals(ResourceInformation.MEMORY_MB.getName())
              || entry.getName().equals(ResourceInformation.VCORES.getName())) {
            continue;
          }

          for (ResourceInformation otherEntry : otherResources) {
            if (entry.getName().equals(otherEntry.getName())) {
              diff = entry.compareTo(otherEntry);
              if (diff != 0) {
                break;
              }
            }
          }
        }{code} it would be more efficient to do this:{code}        for (Entry<String,
Integer> entry :
            ResourceUtils.getResourceTypeIndex().entrySet()) {
          if (entry.getKey().equals(ResourceInformation.MEMORY_MB.getName())
              || entry.getKey().equals(ResourceInformation.VCORES.getName())) {
            continue;
          }

          int index = entry.getValue();

          diff = thisResources[index].compareTo(otherResources[index]);

          if (diff != 0) {
            break;
          }
        }{code}
* {{ResourceUtils.updateReadOnlyResources()}} should be {{updateKnownResources()}} according
to the naming in the latest patch
* {{ResourceUtils.resourceNamesArray}} should be {{knownResourceNamesArray}} to be consistent.
* Is there a reason to have "known" (or "readOnly") in the variable names?  Doesn't seem to
add any useful information.
* {{ResourceUtils.resourceNameToIndex}} should be {{RESOURCE_NAME_TO_INDEX}}
* In {{ResourceUtils.initializeResourcesMap()}}, it seems to me that {{    knownResourceTypes
= Collections.unmodifiableMap(resourceInformationMap);}} should be moved into {{ResourceUtils.updateReadOnlyResources()}}
* The _foreach_ loop in {{ResourceUtils.updateResourceTypeIndex()}} should be a regular _for_
loop, or better yet, fill in the index in the loop in {{updateReadOnlyResources()}}.
* Why doesn't {{ResourceUtils.getResourceNamesArray()}} call {{getResourceTypes()}}?
* The _for_ loop to update resource names in {{ResourceUtils.getResourceTypes()}} is a lovely
opportunity for a lambda. :)
* In the _for_ loop to update resource names in {{ResourceUtils.getResourceTypes()}} memory
and CPU aren't handled a priori, and there's no guarantee that the set iteration will match
the map values iteration in {{updateReadOnlyResources()}}, so the indexing will be off.  If
would be best to just fill in the array in the loop {{ResourceUtils.initializeResourcesMap()}}.
* In the {{FixedValueResource()}} constructor, the {{resources}} array is not created according
to the resource type index, so resource lookups will fail.
* {{FixedValueResource.initResourceMap()}} should populate {{resourceMap}} rather than returning
a map.
* In {{Resources.addTo()}}, {{Resources.subtractFrom()}}, {{Resources.multiplyTo()}}, {{Resources.multiplyAndAddTo()}},{{Resources.multiplyAndRoundDown()}},
{{Resources.fitsIn()}}, {{Resources.componentwiseMin()}}, and {{Resources.componentwiseMax()}},
the variable in the _foreach_ should be named {{lhsValue}} instead of declaring a new variable
for it.
* The catch in {{Resources.addTo()}}, {{Resources.subtractFrom()}}, {{Resources.multiplyAndAddTo()}},
{{Resources.fitsIn()}}, {{Resources.componentwiseMin()}}, and {{Resources.componentwiseMax()}}
should at least log the issue.
* There's now no class that extends {{Resource}} other than {{BaseResource}}.  Maybe it's
time to merge {{BaseResource}} into {{Resource}}?
* Since you're adding the {{package-info.java} file, you should annotate it for audience and
visibility.
* In {{TestResources}}, the import of {{TestResourceUtils}} isn't needed.
* The changes in {{TestResources}} don't appear to do anything.  In {{setupExtraResourceType()}},
the extra resource is added to {{testFile}}, but {{testFile}} is then never used, so adding
the resource has no effect.
* In {{ResourcePBImpl.initResources()}}, the copy to {{readOnlyResources}} should probably
be moved into {{initResourcesMap()}} in place of the empty init.

> Improve performance of resource profile branch
> ----------------------------------------------
>
>                 Key: YARN-6788
>                 URL: https://issues.apache.org/jira/browse/YARN-6788
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: nodemanager, resourcemanager
>            Reporter: Sunil G
>            Assignee: Sunil G
>            Priority: Blocker
>         Attachments: YARN-6788-YARN-3926.001.patch, YARN-6788-YARN-3926.002.patch, YARN-6788-YARN-3926.003.patch,
YARN-6788-YARN-3926.004.patch, YARN-6788-YARN-3926.005.patch, YARN-6788-YARN-3926.006.patch,
YARN-6788-YARN-3926.007.patch, YARN-6788-YARN-3926.008.patch, YARN-6788-YARN-3926.009.patch,
YARN-6788-YARN-3926.010.patch, YARN-6788-YARN-3926.011.patch, YARN-6788-YARN-3926.012.patch,
YARN-6788-YARN-3926.013.patch
>
>
> Currently we could see a 15% performance delta with this branch. 
> Few performance improvements to improve the same.
> Also this patch will handle [comments|https://issues.apache.org/jira/browse/YARN-6761?focusedCommentId=16075418&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16075418]
from [~leftnoteasy].



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org


Mime
View raw message