Skip to content

Commit

Permalink
Changes following review comments
Browse files Browse the repository at this point in the history 
Signed-off-by: Shamser Ahmed <[email protected]>
  • Loading branch information
@shamser
shamser committed Nov 30, 2023
1 parent 9e3b9f4 commit b1b1b2f
Show file tree
Hide file tree
Showing 5 changed files with 38 additions and 47 deletions.
18 changes: 10 additions & 8 deletions dali/base/dadfs.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4953,17 +4953,19 @@ protected: friend class CDistributedFilePart;
__int64 numDiskWrites = 0, numDiskReads = 0;
if (attrs)
{
if (hasReadWriteCostFields(attrs))
if (hasReadWriteCostFields(*attrs))
{
// Newer files have readCost and writeCost attributes
accessCost = cost_type2money(attrs->getPropInt64(getDFUQResultFieldName(DFUQRFreadCost)) + attrs->getPropInt64(getDFUQResultFieldName(DFUQRFwriteCost)));
}
else
{
// (Costs needs to be calculated from numDiskReads and numDiskWrites for legacy files)
// Costs need to be calculated from numDiskReads and numDiskWrites for legacy files
numDiskWrites = attrs->getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskWrites));
numDiskReads = attrs->getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads));
doLegacyAccessCostCalc = true;
// But legacy index files numDiskReads is likely to be widely inaccurate, so don't use it
if (!isFileKey(*attrs))
numDiskReads = attrs->getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads));
}
}
if (isEmptyString(cluster))
Expand Down Expand Up @@ -13447,17 +13449,17 @@ IPropertyTreeIterator *deserializeFileAttrIterator(MemoryBuffer& mb, unsigned nu
sizeDiskSize = file->getPropInt64(getDFUQResultFieldName(DFUQRForigsize), 0);
double sizeGB = sizeDiskSize / ((double)1024 * 1024 * 1024);
double accessCost = 0.0;
if (hasReadWriteCostFields(file))
if (hasReadWriteCostFields(*file))
{
// Newer files have readCost and writeCost attributes
accessCost = cost_type2money(file->getPropInt64(getDFUQResultFieldName(DFUQRFreadCost)) + file->getPropInt64(getDFUQResultFieldName(DFUQRFwriteCost)));
}
else
else // Calc access cost from numDiskRead & numDiskWrites for Legacy files
{
// (Costs needs to be calculated from numDiskReads and numDiskWrites for legacy files)
cost_type legacyReadCost = getLegacyReadCost(*file, nodeGroup);
__int64 numDiskWrites = file->getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskWrites), 0);
__int64 numDiskReads = file->getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads), 0);
accessCost = calcFileAccessCost(nodeGroup, numDiskWrites, numDiskReads);

accessCost = legacyReadCost + calcFileAccessCost(nodeGroup, numDiskWrites, 0);
}
double atRestCost = calcFileAtRestCost(nodeGroup, sizeGB, fileAgeDays);
file->setPropReal(getDFUQResultFieldName(DFUQRFcost), atRestCost+accessCost);
Expand Down
21 changes: 18 additions & 3 deletions dali/base/dadfs.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -863,7 +863,7 @@ extern da_decl GroupType translateGroupType(const char *groupType);

// Useful property query functions

inline bool isFileKey(IPropertyTree &pt) { const char *kind = pt.queryProp("@kind"); return kind&&strieq(kind,"key"); }
inline bool isFileKey(const IPropertyTree &pt) { const char *kind = pt.queryProp("@kind"); return kind&&strieq(kind,"key"); }
inline bool isFileKey(IDistributedFile *f) { return isFileKey(f->queryAttributes()); }
inline bool isFileKey(IFileDescriptor *f) { return isFileKey(f->queryProperties()); }

Expand Down Expand Up @@ -896,8 +896,23 @@ constexpr bool defaultPrivilegedUser = true;
constexpr bool defaultNonPrivilegedUser = false;

extern da_decl void configurePreferredPlanes();
inline bool hasReadWriteCostFields(const IPropertyTree *pt)
inline bool hasReadWriteCostFields(const IPropertyTree & fileAttr)
{
return pt->hasProp(getDFUQResultFieldName(DFUQRFreadCost)) || pt->hasProp(getDFUQResultFieldName(DFUQRFwriteCost));
return fileAttr.hasProp(getDFUQResultFieldName(DFUQRFreadCost)) || fileAttr.hasProp(getDFUQResultFieldName(DFUQRFwriteCost));
}

template<typename Source>
inline cost_type getLegacyReadCost(const IPropertyTree & fileAttr, Source source)
{
// Legacy files do not have @readCost attribute, so calculate from numDiskRead
// But don't try to calc legacy cost if index file because numDiskReads is likely to be inaccurate
if (!hasReadWriteCostFields(fileAttr) && fileAttr.hasProp(getDFUQResultFieldName(DFUQRFnumDiskReads))
&& !isFileKey(fileAttr))
{
stat_type prevDiskReads = fileAttr.getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads), 0);
return money2cost_type(calcFileAccessCost(source, 0, prevDiskReads));
}
else
return 0;
}
#endif
14 changes: 3 additions & 11 deletions dali/ft/filecopy.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3776,17 +3776,9 @@ void FileSprayer::updateTargetProperties()
if (distributedSource->querySuperFile()==nullptr)
{
IPropertyTree & fileAttr = distributedSource->queryAttributes();
cost_type readCost = 0;
// Check if it is a legacy file without @readCost attribute -> calculate prevReadCost
if (!hasReadWriteCostFields(&fileAttr) && fileAttr.hasProp(getDFUQResultFieldName(DFUQRFnumDiskReads)))
{
stat_type totalReads = fileAttr.getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads), 0) + totalNumReads;
readCost = money2cost_type(calcFileAccessCost(distributedSource, 0, totalReads));
}
else
readCost = money2cost_type(calcFileAccessCost(distributedSource, 0, totalNumReads));

distributedSource->addAttrValue(getDFUQResultFieldName(DFUQRFreadCost), readCost);
cost_type legacyReadCost = getLegacyReadCost(fileAttr, distributedSource);
cost_type curReadCost = money2cost_type(calcFileAccessCost(distributedSource, 0, totalNumReads));
distributedSource->addAttrValue(getDFUQResultFieldName(DFUQRFreadCost), legacyReadCost+curReadCost);
distributedSource->addAttrValue(getDFUQResultFieldName(DFUQRFnumDiskReads), totalNumReads);
}
}
Expand Down
17 changes: 4 additions & 13 deletions ecl/hthor/hthor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8541,20 +8541,11 @@ void CHThorDiskReadBaseActivity::closepart()
dFile = &(super->querySubFile(subfile, true));
}
}
StringBuffer clusterName;
dFile->getClusterName(0, clusterName);
cost_type readCost = 0;
IPropertyTree & fileAttr = dFile->queryAttributes();
// Legacy files will not have the readCost stored as an attribute
if (!hasReadWriteCostFields(&fileAttr) && fileAttr.hasProp(getDFUQResultFieldName(DFUQRFnumDiskReads)))
{
// Legacy file: calculate readCost using prev disk reads and new disk reads
stat_type prevDiskReads = fileAttr.getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads), 0);
readCost = money2cost_type(calcFileAccessCost(clusterName, 0, prevDiskReads+curDiskReads));
}
else
readCost = money2cost_type(calcFileAccessCost(clusterName, 0, curDiskReads));
dFile->addAttrValue(getDFUQResultFieldName(DFUQRFreadCost), readCost);
cost_type legacyReadCost = getLegacyReadCost(fileAttr, dFile);
cost_type curReadCost = money2cost_type(calcFileAccessCost(dFile, 0, curDiskReads));

dFile->addAttrValue(getDFUQResultFieldName(DFUQRFreadCost), legacyReadCost + curReadCost);
dFile->addAttrValue(getDFUQResultFieldName(DFUQRFnumDiskReads), curDiskReads);
}
numDiskReads += curDiskReads;
Expand Down
15 changes: 3 additions & 12 deletions thorlcr/graph/thgraphmaster.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -653,18 +653,9 @@ void CMasterActivity::updateFileReadCostStats()
auto updateReadCosts = [](IDistributedFile *file, CThorStatsCollection &stats)
{
stat_type curDiskReads = stats.getStatisticSum(StNumDiskReads);
StringBuffer clusterName;
file->getClusterName(0, clusterName);
cost_type legacyReadCost = 0, curReadCost = 0;
IPropertyTree & fileAttr = file->queryAttributes();
// Legacy files will not have the readCost stored as an attribute
if (!hasReadWriteCostFields(&fileAttr) && fileAttr.hasProp(getDFUQResultFieldName(DFUQRFnumDiskReads)))
{
// Legacy file: calculate readCost using prev disk reads and new disk reads
stat_type prevDiskReads = fileAttr.getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads), 0);
legacyReadCost = money2cost_type(calcFileAccessCost(clusterName, 0, prevDiskReads));
}
curReadCost = money2cost_type(calcFileAccessCost(clusterName, 0, curDiskReads));
cost_type legacyReadCost = getLegacyReadCost(fileAttr, file);
cost_type curReadCost = money2cost_type(calcFileAccessCost(file, 0, curDiskReads));
file->addAttrValue(getDFUQResultFieldName(DFUQRFreadCost), legacyReadCost + curReadCost);
file->addAttrValue(getDFUQResultFieldName(DFUQRFnumDiskReads), curDiskReads);
return curReadCost;
Expand Down Expand Up @@ -715,7 +706,7 @@ void CMasterActivity::updateFileWriteCostStats(IFileDescriptor & fileDesc, IProp
fileDesc.getClusterGroupName(0, clusterName);// Note: calculating for 1st cluster. (Future: calc for >1 clusters)
cost_type writeCost = money2cost_type(calcFileAccessCost(clusterName, numDiskWrites, 0));
props.setPropInt64(getDFUQResultFieldName(DFUQRFwriteCost), writeCost);
diskAccessCost += writeCost;
diskAccessCost = writeCost;
}
}

Expand Down

0 comments on commit b1b1b2f

Please sign in to comment.