[
https://issues.apache.org/jira/browse/HADOOP-17408?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17259194#comment-17259194
]
Jim Brennan commented on HADOOP-17408:
--------------------------------------
[~ahussein] thanks for the PR! Can you please separate this into two parts?
I would like to see a separate Jira/PR with just the changes that [~daryn] made
internally - those changes have gotten some run-time and are a clear
optimization.
The additional changes you have made are mostly a refactoring, and I am not
convinced the original behavior has been retained. Optimizing away the
shuffle could have been achieved by just moving the shuffle into the else case:
{noformat}
if (secondarySort != null) {
secondarySort.accept(list);
} else {
Collections.shuffle(list, r);
}
{noformat}
The other concern with the refactoring portion is that it changes the signature
of public method sortByDistance().
> Optimize NetworkTopology while sorting of block locations
> ---------------------------------------------------------
>
> Key: HADOOP-17408
> URL: https://issues.apache.org/jira/browse/HADOOP-17408
> Project: Hadoop Common
> Issue Type: Improvement
> Components: common, net
> Reporter: Ahmed Hussein
> Assignee: Ahmed Hussein
> Priority: Major
> Labels: pull-request-available
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> In {{NetworkTopology}}, I noticed that there are some hanging fruits to
> improve the performance.
> Inside {{sortByDistance}}, collections.shuffle is performed on the list
> before calling {{secondarySort}}.
> {code:java}
> Collections.shuffle(list, r);
> if (secondarySort != null) {
> secondarySort.accept(list);
> }
> {code}
> However, in different call sites, {{collections.shuffle}} is passed as the
> secondarySort to {{sortByDistance}}. This means that the shuffle is executed
> twice on each list.
> Also, logic wise, it is useless to shuffle before applying a tie breaker
> which might make the shuffle work obsolete.
> In addition, [~daryn] reported that:
> * topology is unnecessarily locking/unlocking to calculate the distance for
> every node
> * shuffling uses a seeded Random, instead of ThreadLocalRandom, which is
> heavily synchronized
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
