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-32880 Ensure that legacy write costs are not lost after file read operation #19286

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

HPCC-32880 Ensure that legacy write costs are not lost after file read operation #19286

Show file tree
Hide file tree
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
78 changes: 32 additions & 46 deletions dali/base/dadfs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,27 @@ extern da_decl cost_type calcDiskWriteCost(const StringArray & clusters, stat_ty
return writeCost;
}


extern da_decl cost_type updateCostAndNumReads(IDistributedFile *file, stat_type numDiskReads, cost_type curReadCost)
{
const IPropertyTree & fileAttr = file->queryAttributes();

if (!curReadCost)
curReadCost = calcFileAccessCost(file, 0, numDiskReads);
cost_type legacyReadCost = 0;
if (!fileAttr.hasProp(getDFUQResultFieldName(DFUQRFreadCost)))
{
if (!isFileKey(fileAttr))
{
stat_type prevDiskReads = fileAttr.getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads), 0);
legacyReadCost = calcFileAccessCost(file, 0, prevDiskReads);
}
}
file->addAttrValue(getDFUQResultFieldName(DFUQRFreadCost), legacyReadCost + curReadCost);
file->addAttrValue(getDFUQResultFieldName(DFUQRFnumDiskReads), numDiskReads);
return curReadCost;
}

// JCSMORE - I suspect this function should be removed/deprecated. It does not deal with dirPerPart or striping.
// makePhysicalPartName supports both, but does not deal with groups/endpoints)
RemoteFilename &constructPartFilename(IGroup *grp,unsigned partno,unsigned partmax,const char *name,const char *partmask,const char *partdir,unsigned copy,ClusterPartDiskMapSpec &mspec,RemoteFilename &rfn)
Expand Down Expand Up @@ -4985,49 +5006,29 @@ protected: friend class CDistributedFilePart;
}
virtual void getCost(const char * cluster, cost_type & atRestCost, cost_type & accessCost) override
{
atRestCost = 0;
accessCost = 0;
CDateTime dt;
getModificationTime(dt);
double fileAgeDays = difftime(time(nullptr), dt.getSimple())/(24*60*60);
double sizeGB = getDiskSize(true, false) / ((double)1024 * 1024 * 1024);
const IPropertyTree *attrs = root->queryPropTree("Attr");
bool doLegacyAccessCostCalc = false;
__int64 numDiskWrites = 0, numDiskReads = 0;
if (attrs)
{
if (hasReadWriteCostFields(*attrs))
{
// Newer files have readCost and writeCost attributes
accessCost = attrs->getPropInt64(getDFUQResultFieldName(DFUQRFreadCost)) + attrs->getPropInt64(getDFUQResultFieldName(DFUQRFwriteCost));
}
else
{
// Costs need to be calculated from numDiskReads and numDiskWrites for legacy files
numDiskWrites = attrs->getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskWrites));
doLegacyAccessCostCalc = true;
// NB: Costs of index reading can not be reliably estimated based on 'numDiskReads'
if (!isFileKey(*attrs))
numDiskReads = attrs->getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads));
}
}

if (isEmptyString(cluster))
{
StringArray clusterNames;
unsigned countClusters = getClusterNames(clusterNames);
for (unsigned i = 0; i < countClusters; i++)
atRestCost += calcFileAtRestCost(clusterNames[i], sizeGB, fileAgeDays);
if (countClusters && doLegacyAccessCostCalc)
{
// NB: numDiskReads/numDiskWrites are stored at the file level, not per cluster.
// So cannot calculate accessCost per cluster, assume cost is based on 1st.
accessCost = calcFileAccessCost(clusterNames[0], numDiskWrites, numDiskReads);
}
if (countClusters)
cluster = clusterNames[0];
}
else
{
atRestCost += calcFileAtRestCost(cluster, sizeGB, fileAgeDays);
if (doLegacyAccessCostCalc)
accessCost = calcFileAccessCost(cluster, numDiskWrites, numDiskReads);
atRestCost = calcFileAtRestCost(cluster, sizeGB, fileAgeDays);
}
if (attrs)
jakesmith marked this conversation as resolved.
Show resolved Hide resolved
accessCost = getReadCost(*attrs, cluster) + getWriteCost(*attrs, cluster);
}
};

Expand Down Expand Up @@ -13456,25 +13457,10 @@ IPropertyTreeIterator *deserializeFileAttrIterator(MemoryBuffer& mb, unsigned nu
cost_type atRestCost = calcFileAtRestCost(nodeGroup, sizeGB, fileAgeDays);
file->setPropInt64(getDFUQResultFieldName(DFUQRFatRestCost), atRestCost);

// Dyamically calc and set the access cost field and for legacy files set read/write cost fields
cost_type accessCost = 0;
if (hasReadWriteCostFields(*file))
{
accessCost = file->getPropInt64(getDFUQResultFieldName(DFUQRFreadCost)) + file->getPropInt64(getDFUQResultFieldName(DFUQRFwriteCost));
}
else // Calc access cost from numDiskRead & numDiskWrites for Legacy files
{
cost_type legacyReadCost = getLegacyReadCost(*file, nodeGroup);
file->setPropInt64(getDFUQResultFieldName(DFUQRFreadCost), legacyReadCost);

cost_type legacyWriteCost = getLegacyWriteCost(*file, nodeGroup);
file->setPropInt64(getDFUQResultFieldName(DFUQRFwriteCost), legacyWriteCost);

accessCost = legacyReadCost + legacyWriteCost;
}
cost_type readCost = getReadCost(*file, nodeGroup, true);
cost_type writeCost = getWriteCost(*file, nodeGroup, true);
cost_type accessCost = readCost + writeCost;
file->setPropInt64(getDFUQResultFieldName(DFUQRFaccessCost), accessCost);

// Dymically calc and set the total cost field
file->setPropInt64(getDFUQResultFieldName(DFUQRFcost), atRestCost + accessCost);
}

Expand Down
88 changes: 71 additions & 17 deletions dali/base/dadfs.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -890,39 +890,93 @@ extern da_decl cost_type calcFileAtRestCost(const char * cluster, double sizeGB,
extern da_decl cost_type calcFileAccessCost(const char * cluster, __int64 numDiskWrites, __int64 numDiskReads);
extern da_decl cost_type calcFileAccessCost(IDistributedFile *f, __int64 numDiskWrites, __int64 numDiskReads);
extern da_decl cost_type calcDiskWriteCost(const StringArray & clusters, stat_type numDiskWrites);
extern da_decl cost_type updateCostAndNumReads(IDistributedFile *file, stat_type numDiskReads, cost_type curReadCost=0); // Update readCost and numDiskReads - return calculated read cost
constexpr bool defaultPrivilegedUser = true;
constexpr bool defaultNonPrivilegedUser = false;

extern da_decl void configurePreferredPlanes();
inline bool hasReadWriteCostFields(const IPropertyTree & fileAttr)

// Get read cost from readCost field or calculate legacy read cost
// - migrateLegacyCost: if true, update readCost field with legacy read cost
template<typename Source>
inline cost_type getReadCost(IPropertyTree & fileAttr, Source source, bool migrateLegacyCost = false)
{
return fileAttr.hasProp(getDFUQResultFieldName(DFUQRFreadCost)) || fileAttr.hasProp(getDFUQResultFieldName(DFUQRFwriteCost));
if (fileAttr.hasProp(getDFUQResultFieldName(DFUQRFreadCost)))
return fileAttr.getPropInt64(getDFUQResultFieldName(DFUQRFreadCost), 0);
else
{
// Calculate legacy read cost from numDiskReads
// (However, it is not possible to accurately calculate read cost for key
// files, as the reads may have been from page cache and not from disk.)
if (!isFileKey(fileAttr) && source)
{
stat_type numDiskReads = fileAttr.getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads), 0);
cost_type readCost = calcFileAccessCost(source, 0, numDiskReads);
if (migrateLegacyCost)
fileAttr.setPropInt64(getDFUQResultFieldName(DFUQRFreadCost), readCost);
return readCost;
}
}
return 0;
}

// Get read cost from readCost field or calculate legacy read cost
template<typename Source>
inline cost_type getLegacyReadCost(const IPropertyTree & fileAttr, Source source)
inline cost_type getReadCost(const IPropertyTree & fileAttr, Source source)
{
// Legacy files do not have @readCost attribute, so calculate from numDiskRead
// NB: Costs of index reading can not be reliably estimated based on 'numDiskReads'
if (!hasReadWriteCostFields(fileAttr) && fileAttr.hasProp(getDFUQResultFieldName(DFUQRFnumDiskReads))
&& !isFileKey(fileAttr))
if (fileAttr.hasProp(getDFUQResultFieldName(DFUQRFreadCost)))
return fileAttr.getPropInt64(getDFUQResultFieldName(DFUQRFreadCost), 0);
else
{
stat_type prevDiskReads = fileAttr.getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads), 0);
return calcFileAccessCost(source, 0, prevDiskReads);
// Calculate legacy read cost from numDiskReads
// (However, it is not possible to accurately calculate read cost for key
// files, as the reads may have been from page cache and not from disk.)
if (!isFileKey(fileAttr) && source)
{
stat_type numDiskReads = fileAttr.getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads), 0);
return calcFileAccessCost(source, 0, numDiskReads);
}
}
else
return 0;
return 0;
}

// Get write cost from writeCost field or calculate legacy write cost
// - migrateLegacyCost: if true, update writeCost field with legacy write cost
template<typename Source>
inline cost_type getLegacyWriteCost(const IPropertyTree & fileAttr, Source source)
inline cost_type getWriteCost(IPropertyTree & fileAttr, Source source, bool migrateLegacyCost = false)
{
// Legacy files do not have @writeCost attribute, so calculate from numDiskWrites
if (!hasReadWriteCostFields(fileAttr) && fileAttr.hasProp(getDFUQResultFieldName(DFUQRFnumDiskWrites)))
if (fileAttr.hasProp(getDFUQResultFieldName(DFUQRFwriteCost)))
return fileAttr.getPropInt64(getDFUQResultFieldName(DFUQRFwriteCost), 0);
else
{
stat_type prevDiskWrites = fileAttr.getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskWrites), 0);
return calcFileAccessCost(source, prevDiskWrites, 0);
// Calculate legacy write cost from numDiskWrites
if (source)
{
stat_type numDiskWrites = fileAttr.getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskWrites), 0);
cost_type writeCost = calcFileAccessCost(source, numDiskWrites, 0);
if (migrateLegacyCost)
fileAttr.setPropInt64(getDFUQResultFieldName(DFUQRFwriteCost), writeCost);
return writeCost;
}
}
return 0;
}

// Get write cost from writeCost field or calculate legacy write cost
template<typename Source>
inline cost_type getWriteCost(const IPropertyTree & fileAttr, Source source)
{
if (fileAttr.hasProp(getDFUQResultFieldName(DFUQRFwriteCost)))
return fileAttr.getPropInt64(getDFUQResultFieldName(DFUQRFwriteCost), 0);
else
return 0;
{
// Calculate legacy write cost from numDiskWrites
if (source)
{
stat_type numDiskWrites = fileAttr.getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskWrites), 0);
return calcFileAccessCost(source, numDiskWrites, 0);
}
}
return 0;
}
#endif
15 changes: 4 additions & 11 deletions dali/ft/filecopy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3851,10 +3851,10 @@ cost_type FileSprayer::updateSourceProperties()
// Update file readCost and numReads in file properties and do the same for subfiles
if (distributedSource)
{
cost_type totalReadCost = 0;
IDistributedSuperFile * superSrc = distributedSource->querySuperFile();
if (superSrc && superSrc->numSubFiles() > 0)
{
cost_type totalReadCost = 0;
Owned<IFileDescriptor> fDesc = superSrc->getFileDescriptor();
ISuperFileDescriptor *superFDesc = fDesc->querySuperFileDescriptor();
ForEachItemIn(idx, partition)
Expand All @@ -3876,27 +3876,20 @@ cost_type FileSprayer::updateSourceProperties()
// so query the first (and only) subfile
subfile = &superSrc->querySubFile(0);
}
cost_type curReadCost = calcFileAccessCost(subfile, 0, curProgress.numReads);
subfile->addAttrValue(getDFUQResultFieldName(DFUQRFnumDiskReads), curProgress.numReads);
cost_type legacyReadCost = getLegacyReadCost(subfile->queryAttributes(), subfile);
subfile->addAttrValue(getDFUQResultFieldName(DFUQRFreadCost), legacyReadCost + curReadCost);
totalReadCost += curReadCost;
totalReadCost += updateCostAndNumReads(subfile, curProgress.numReads);
}
else
{
// not sure if src superfile can have whichInput==-1 (but if so, this is best effort to calc cost)
totalReadCost += calcFileAccessCost(distributedSource, 0, curProgress.numReads);
}
}
return updateCostAndNumReads(distributedSource, totalNumReads, totalReadCost);
Copy link
Member

Choose a reason for hiding this comment

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

Probably reasonable to set these super props at this time, but they're going to be wrong if/when the super file is manipulated, e.g. a new subfile added or removed.

I think updateFileAttrs needs to be updated to cope with that.
They're also going to be wrong after o single subfile in an existing super is read ? i.e. what would update the super cost under those circumstances?

When the engines (e.g. thor) reads a super, it updates the sub file read costs in CMasterActivity::calcFileReadCostStats, I can't see that it updates the super costs though?

This PR may be okay as it is, but looks like there are problems with super cost info going stale in general?
@shamser - please comment, and if appropriate, open a new JIRA, reference here, and link it to this JIRA.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A jira to revisit super file costs: https://hpccsystems.atlassian.net/browse/HPCC-32901

}
else
{
totalReadCost = calcFileAccessCost(distributedSource, 0, totalNumReads);
return updateCostAndNumReads(distributedSource, totalNumReads);
}
distributedSource->addAttrValue(getDFUQResultFieldName(DFUQRFnumDiskReads), totalNumReads);
cost_type legacyReadCost = getLegacyReadCost(distributedSource->queryAttributes(), distributedSource);
distributedSource->addAttrValue(getDFUQResultFieldName(DFUQRFreadCost), legacyReadCost + totalReadCost);
return totalReadCost; // return the total cost of this file operation (exclude previous and legacy read costs)
}
return 0;
}
Expand Down
7 changes: 1 addition & 6 deletions ecl/hthor/hthor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8547,12 +8547,7 @@ void CHThorDiskReadBaseActivity::closepart()
dFile = &(super->querySubFile(subfile, true));
}
}
IPropertyTree & fileAttr = dFile->queryAttributes();
cost_type legacyReadCost = getLegacyReadCost(fileAttr, dFile);
cost_type curReadCost = calcFileAccessCost(dFile, 0, curDiskReads);

dFile->addAttrValue(getDFUQResultFieldName(DFUQRFreadCost), legacyReadCost + curReadCost);
dFile->addAttrValue(getDFUQResultFieldName(DFUQRFnumDiskReads), curDiskReads);
updateCostAndNumReads(dFile, curDiskReads);
}
numDiskReads += curDiskReads;
}
Expand Down
14 changes: 1 addition & 13 deletions thorlcr/graph/thgraphmaster.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,7 +657,6 @@ cost_type CMasterActivity::calcFileReadCostStats(bool updateFileProps)
{
StringBuffer clusterName;
file->getClusterName(0, clusterName);
IPropertyTree & fileAttr = file->queryAttributes();
cost_type curReadCost = 0;
stat_type curDiskReads = stats.getStatisticSum(StNumDiskReads);
if(useJhtreeCacheStats)
Expand All @@ -671,18 +670,7 @@ cost_type CMasterActivity::calcFileReadCostStats(bool updateFileProps)
curReadCost = calcFileAccessCost(clusterName, 0, curDiskReads);

if (updateFileProps)
{
cost_type legacyReadCost = 0;
// 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 = calcFileAccessCost(clusterName, 0, prevDiskReads);
}
file->addAttrValue(getDFUQResultFieldName(DFUQRFreadCost), legacyReadCost + curReadCost);
file->addAttrValue(getDFUQResultFieldName(DFUQRFnumDiskReads), curDiskReads);
}
updateCostAndNumReads(file, curDiskReads);
return curReadCost;
};
cost_type readCost = 0;
Expand Down
Loading