Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HPCC-31062 Include dfu file op cost in workunit aggregates #18358

Merged
merged 1 commit into from
May 9, 2024
+25 −5
Merged

HPCC-31062 Include dfu file op cost in workunit aggregates #18358

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 25 additions & 5 deletions common/workunit/workunit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2681,13 +2681,12 @@ void StatisticsAggregator::loadExistingAggregates(const IConstWorkUnit &workunit
};

WuScopeFilter filter;
filter.addScopeType(SSTglobal).addScopeType(SSTworkflow).addScopeType(SSTgraph);
filter.addScopeType(SSTglobal).addScopeType(SSTworkflow).addScopeType(SSTgraph).addScopeType(SSToperation);
const unsigned numStats = mapping.numStatistics();
for (unsigned i=0; i<numStats; ++i)
filter.addOutputStatistic(mapping.getKind(i));
filter.setDepth(1,3); // 1=global, 2=workflow, 3=graph
filter.setDepth(0,3); // 0=global, 1=workflow, (2=graph, 3=subgraph | 2=>dfu)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you probably just want to delete this line. As long as you have the next line it will efficiently walk the existing global stats.

Copy link
Contributor Author

@shamser shamser May 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By "global", I mean the top-level scopes (e.g. parent of the workflow scopes). To iterate through the top level scopes, the depth needs to start at 0. Here is information from logging noteStatistic to record the scope names (depth 0,3):

noteStatistic Statistic CostExecute (value 415263369)
noteStatistic Statistic CostFileAccess (value 12554)
noteStatistic w1 Statistic CostExecute (value 415263369)
noteStatistic w1 Statistic CostFileAccess (value 12554)
noteStatistic w1:graph1 Statistic CostExecute (value 386667103)
noteStatistic w1:graph2 Statistic CostExecute (value 266666968)
noteStatistic w1:graph3 Statistic CostExecute (value 4056896594)
noteStatistic w1:graph3 Statistic CostFileAccess (value 323)
noteStatistic w1:graph4 Statistic CostFileAccess (value 525)
noteStatistic w1:graph4 Statistic CostExecute (value 3493194855)
noteStatistic w1:>dfu Statistic CostFileAccess (value 11706)
noteStatistic w1:>dfu:>copy Statistic CostFileAccess (value 11706)

If the depth is set to (1,3), the iterator skips the top-level and starts one level down (e.g. at the workflow scope level). Here's the results of the same workunit but with the depth set to (1,3):

noteStatistic w1 Statistic CostExecute (value 941930630)
noteStatistic w1 Statistic CostFileAccess (value 12554)
noteStatistic w1:graph1 Statistic CostExecute (value 384444878)
noteStatistic w1:graph2 Statistic CostExecute (value 246666945)
noteStatistic w1:graph3 Statistic CostExecute (value 310818807)
noteStatistic w1:graph3 Statistic CostFileAccess (value 323)
noteStatistic w1:graph4 Statistic CostFileAccess (value 525)
noteStatistic w1:graph4 Statistic CostExecute (value 1056007436)
noteStatistic w1:>dfu Statistic CostFileAccess (value 11706)
noteStatistic w1:>dfu:>copy Statistic CostFileAccess (value 11706)

I changed to 0,3 because I was seeing that the top-level totals wasn't being updated. With this change, they are.
@ghalliday

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think you need to change from 1,3 to 0,3. I think you could equally delete the filter, but I think probably no harm in leaving it here either.
The intent of the filters was that the sources etc. would be automatically deduced from the filter conditions. That logic needs updating for it to be true - and in this case the source has been explicitly selected.

filter.setSources(SSFsearchGlobalStats);
filter.setIncludeNesting(0);
filter.finishedFilter();

StatsCollectionAggregatesLoader aggregatesLoader(statsCollection);
Expand Down Expand Up @@ -8941,7 +8940,7 @@ void CLocalWorkUnit::setStatistic(StatisticCreatorType creatorType, const char *
if (mergeAction != StatsMergeAppend)
{
StringBuffer xpath;
xpath.append("Statistic[@creator='").append(creator).append("'][@scope='").append(scope).append("'][@kind='").append(kindName).append("']");
xpath.append("Statistic[@scope='").append(scope).append("'][@kind='").append(kindName).append("']");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks correct, but what was the reason for it? The jira or the PR should include some indication of why.

statTree = stats->queryPropTree(xpath.str());
}

Expand Down Expand Up @@ -8969,6 +8968,7 @@ void CLocalWorkUnit::setStatistic(StatisticCreatorType creatorType, const char *
mergeAction = StatsMergeAppend;
}

unsigned __int64 deltaValue = 0;
if (mergeAction != StatsMergeAppend) // RKC->GH Is this right??
{
unsigned __int64 oldValue = statTree->getPropInt64("@value", 0);
Expand All @@ -8977,14 +8977,18 @@ void CLocalWorkUnit::setStatistic(StatisticCreatorType creatorType, const char *
if (oldMax < oldValue)
oldMax = oldValue;

statTree->setPropInt64("@value", mergeStatisticValue(oldValue, value, mergeAction));
unsigned __int64 newValue = mergeStatisticValue(oldValue, value, mergeAction);
statTree->setPropInt64("@value", newValue);
statTree->setPropInt64("@count", count + oldCount);
if (maxValue > oldMax)
statTree->setPropInt64("@max", maxValue);
deltaValue = newValue - oldValue;
}
else
{
statTree->setPropInt64("@value", value);
deltaValue = value;

statTree->setPropInt64("@count", count);
if (maxValue)
statTree->setPropInt64("@max", maxValue);
Expand Down Expand Up @@ -9016,6 +9020,22 @@ void CLocalWorkUnit::setStatistic(StatisticCreatorType creatorType, const char *
}
if (kind == StCostCompile)
p->setPropInt64("@costCompile", value);

// Special case - update aggregates for dfu FileAccessCost. This is needed because although
// fileservices can update dfu cost in the workunit, it does not have a mechanism to update
// the aggregates for this stat.
if (scopeType == SSTdfuworkunit && kind == StCostFileAccess && deltaValue)
{
StringBuffer currentScope(scope), parent;
while (getParentScope(parent.clear(), currentScope.str()))
{
currentScope.set(parent);
StatisticScopeType sst = getScopeType(queryScopeTail(currentScope.str()));
if (sst!=SSTnone)
setStatistic(creatorType, creator, sst, currentScope.str(), StCostFileAccess, "", deltaValue, 1, 0, StatsMergeSum);
}
setStatistic(creatorType, creator, SSTglobal, "", StCostFileAccess, "", deltaValue, 1, 0, StatsMergeSum);
}
}

void CLocalWorkUnit::_loadStatistics() const
Expand Down
Loading