HPCC-30911 Store read and write cost as file attributes #18080

shamser · 2023-11-24T12:33:39Z

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

github-actions · 2023-11-24T13:11:23Z

https://track.hpccsystems.com/browse/HPCC-30911
Jira updated

jakesmith

@shamser - looks good in general, mostly 'future' comments, but 1 issue that needs fixing.

dali/base/dadfs.cpp

dali/ft/filecopy.cpp

thorlcr/graph/thgraphmaster.cpp

jakesmith

@shamser - looks good, aside from opening a related JIRA for : #18080 (comment)

Please add or link the JIRA and squash the commits.

ghalliday

@shamser generally looks good. A couple of bits of code could be commoned up, but may not be worth it for this PR.
Concern that the correct cluster is used when calculating read/write costs, and that legacy costs for indexes may be wild over-estimates.

ghalliday · 2023-11-29T14:11:34Z

dali/base/dadfs.cpp

+                // (Costs needs to be calculated from numDiskReads and numDiskWrites for legacy files)
+                numDiskWrites = attrs->getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskWrites));
+                numDiskReads = attrs->getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads));
+                doLegacyAccessCostCalc = true;


Possibly only for files, not indexes if the index stats are currently wildly wrong.

ghalliday · 2023-11-29T14:16:35Z

dali/ft/filecopy.cpp

+                readCost = money2cost_type(calcFileAccessCost(distributedSource, 0, totalReads));
+            }
+            else
+                readCost = money2cost_type(calcFileAccessCost(distributedSource, 0, totalNumReads));


question: Is this tied down to a single cluster by this point? Worth defining an expensive plane and a cheap plane in our standard config, and then checking the costs is recorded correctly depending on the source.

The decision to assume that the source was from the first cluster was made as part of HPCC-27388. I recall that there was some discussion and it was agreed that it would be good enough to assume it was from the first cluster. And I think the basis for that decision was that it was assumed that the cost of each plane would be the same. We can see that this assumption may need to revisited. However, this PR was supposed to about storing the read/write cost to ensure that the keyed join read cost calculations are not overwritten by the legacy dynamic calculations. I think it would be worth considering this issue in separate jira:https://track.hpccsystems.com/browse/HPCC-30946

Agreed. I think that at some point the clusters are reordered so that the "best" cluster comes first - but I'm not sure if that will be done at this point. Something to investigate further.

ghalliday · 2023-11-29T14:18:30Z

ecl/hthor/hthor.cpp

+                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);


Similar code is repeated. Consider creating a helper function

ghalliday · 2023-11-29T14:21:53Z

thorlcr/graph/thgraphmaster.cpp

+    {
+        stat_type curDiskReads = stats.getStatisticSum(StNumDiskReads);
+        StringBuffer clusterName;
+        file->getClusterName(0, clusterName);


This should be whichever cluster it was actually read from - it is likely to make a difference in the future.

To revisit this: https://track.hpccsystems.com/browse/HPCC-30946

ghalliday · 2023-11-29T14:25:10Z

thorlcr/graph/thgraphmaster.cpp

-        diskAccessCost = money2cost_type(calcFileAccessCost(clusterName, numDiskWrites, 0));
+        cost_type writeCost = money2cost_type(calcFileAccessCost(clusterName, numDiskWrites, 0));
+        props.setPropInt64(getDFUQResultFieldName(DFUQRFwriteCost), writeCost);
+        diskAccessCost += writeCost;


I think this will over-accumulate the costs. Should be
diskAccessCost = writeCost
otherwise each time it is called all previous costs will be added again.

I believe updateFileWriteCostStats is only ever called once. However, I've changed to set the diskAccessCost. (This is likely to change shortly as part of https://track.hpccsystems.com/browse/HPCC-30937 )

jakesmith

@shamser - noticed a couple of issues which would be best addressed before merged.

dali/base/dadfs.cpp

dali/ft/filecopy.cpp

ecl/hthor/hthor.cpp

thorlcr/graph/thgraphmaster.cpp

ghalliday

@shamser changes look good.
What other jiras need to be opened to ensure that these attributes are available and displayed in eclwatch? They should be linked to the jira for this issue. (I have linked HPCC-30946)

ghalliday · 2023-11-30T13:00:40Z

We should also think about how we can regression test these features.

shamser · 2023-11-30T13:42:37Z

@shamser changes look good. What other jiras need to be opened to ensure that these attributes are available and displayed in eclwatch? They should be linked to the jira for this issue. (I have linked HPCC-30946)

The accessCost is being displayed correctly. This jira is to show read and write cost attributes in eclwatch: https://track.hpccsystems.com/browse/HPCC-30930
@ghalliday

jakesmith

@shamser - please see comments.

jakesmith · 2023-11-30T14:27:06Z

dali/base/dadfs.cpp

+
+                accessCost = legacyReadCost + calcFileAccessCost(nodeGroup, numDiskWrites, 0);
+            }
+            double atRestCost = calcFileAtRestCost(nodeGroup, sizeGB, fileAgeDays);
            file->setPropReal(getDFUQResultFieldName(DFUQRFcost), atRestCost+accessCost);
            file->setPropReal(getDFUQResultFieldName(DFUQRFatRestCost), atRestCost);
            file->setPropReal(getDFUQResultFieldName(DFUQRFaccessCost), accessCost);


hm, baffled, I wrote a comment on last review submission, but can't find it now..

As the code stands (in this PR), "readCost" and "writeCost" are published as top-level file attributes in cost_type format
e.g.:

cost_type curReadCost = money2cost_type(calcFileAccessCost(file, 0, curDiskReads)); file->addAttrValue(getDFUQResultFieldName(DFUQRFreadCost), legacyReadCost + curReadCost);

But then here, in attributes seen by clients using getDFAttributesTreeIterator, it adds "cost", "atRestCost" and "accessCost" which as reals (converted via cost_type2money).
The net result is that all these attributes will be visible to a caller of getDFAttributesTreeIterator, but some will be in one format and some will be in another.

We should make sure they are consistently in the same format.
I'm not sure it matters which format as long as consistent (I suspect ideally they'd remain as cost_type, until a client usage case needs them).
Either change the new "readCost" "writeCost" attributes so they're published in "money" format, and change calculations accordingly.
Or, keep them as cost_type, but then existing code which expects them as 'money' (e.g. getCost) and the esp service code, would need to change.

jakesmith · 2023-11-30T14:30:47Z

dali/base/dadfs.cpp

+                cost_type legacyReadCost = getLegacyReadCost(*file, nodeGroup);
+                __int64 numDiskWrites = file->getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskWrites), 0);
+
+                accessCost = legacyReadCost + calcFileAccessCost(nodeGroup, numDiskWrites, 0);


shouldn't this be:

accessCost = cost_type2money(legacyReadCost) + calcFileAccessCost(nodeGroup, numDiskWrites, 0);

?

jakesmith

@shamser - mostly trivial comments that would not stop this being merged,
but 1 comment worth discussing.

jakesmith · 2023-12-05T12:07:40Z

dali/base/dadfs.cpp

@@ -13434,6 +13434,8 @@ IPropertyTreeIterator *deserializeFileAttrIterator(MemoryBuffer& mb, unsigned nu

        void setCost(IPropertyTree* file, const char *nodeGroup)
        {
+            // Set the following dynamic fields: atRestCost, accessCost, cost and for legacy files: readCost, writeCost
+            // Dyamically calc and set the atRest cost field


is this 2nd comment line helpful? i.e. isn't it covered in 1st 'atRestCost'?

jakesmith · 2023-12-05T12:18:18Z

dali/base/dadfs.cpp

+                // Costs need to be calculated from numDiskReads and numDiskWrites for legacy files
+                numDiskWrites = attrs->getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskWrites));
+                doLegacyAccessCostCalc = true;
+                // But legacy index files numDiskReads is likely to be widely inaccurate, so don't use it


trivial: I guess "numDiskReads is likely to be widely inaccurate" is not quite accurate, numDiskReads should be correct, it's that costs can't be reliably calculated using it in the context of indexes.
Perhaps rephrase to:
// NB: Costs of index reading can not be reliably estimated based on 'numDiskReads'

jakesmith · 2023-12-05T12:19:04Z

dali/base/dadfs.hpp

+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


see prev. comment, perhaps:
// NB: Costs of index reading can not be reliably estimated based on 'numDiskReads'

jakesmith · 2023-12-05T12:34:55Z

dali/base/dadfs.cpp

-    double atRestCost, accessCost;
-    calcFileCost(cluster, 0, 0, numDiskWrites, numDiskReads, atRestCost, accessCost);
-    return accessCost;
+    Owned<IPropertyTree> costPT = getCostPropTree(cluster);


trivial: could be Owned<const IPropertyTree>

jakesmith · 2023-12-05T12:35:06Z

dali/base/dadfs.cpp

+
+extern da_decl double calcFileAtRestCost(const char * cluster, double sizeGB, double fileAgeDays)
+{
+    Owned<IPropertyTree> costPT = getCostPropTree(cluster);


trivial: could be Owned<const IPropertyTree>

jakesmith · 2023-12-05T12:52:53Z

dali/base/dadfs.hpp

+        && !isFileKey(fileAttr))
+    {
+        stat_type prevDiskReads = fileAttr.getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads), 0);
+        return  money2cost_type(calcFileAccessCost(source, 0, prevDiskReads));


trivial/formatting: double space after 'return'

jakesmith · 2023-12-05T12:54:14Z

dali/base/dadfs.hpp

+    if (!hasReadWriteCostFields(fileAttr) && fileAttr.hasProp(getDFUQResultFieldName(DFUQRFnumDiskWrites)))
+    {
+        stat_type prevDiskWrites = fileAttr.getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskWrites), 0);
+        return  money2cost_type(calcFileAccessCost(source, prevDiskWrites, 0));


trivial/formatting: double space after 'return'

jakesmith · 2023-12-05T13:03:32Z

dali/ft/filecopy.cpp

@@ -3771,7 +3774,13 @@ void FileSprayer::updateTargetProperties()
    if (distributedSource)
    {
        if (distributedSource->querySuperFile()==nullptr)


not new, but what if it is a super being read?
Also not new, but it's misleading that this code to update distributedSource is in a method called "updateTargetProperties". For clarity (now in this PR), I think it should be moved outside of it.

If it is super file, I believe it uses the aggregate of the cost from all the subfiles.

per offline conversation - to be investigated.
What happens when 'distributedSource' is a superfile (is it even supported?). If it is, what happens (if anything currently) re. updating cost attributes on the subfiles read.
@shamser - I've opened a subtask for this (HPCC-30970), under HPCC-30965

jakesmith · 2023-12-05T13:12:30Z

esp/services/ws_dfu/ws_dfuHelpers.cpp

-                lFile->setCost(file.getPropReal(getDFUQResultFieldName(DFUQRFcost)));
+            {
+                cost_type cost = file.getPropInt64(getDFUQResultFieldName(DFUQRFcost));
+                lFile->setCost(cost_type2money(cost));


note to myself - there is no backward compatibility issue here, because DFUQRFcost has been dynamically calculated in this version, via getDFAttributesTreeIterator

jakesmith · 2023-12-05T13:26:40Z

thorlcr/graph/thgraphmaster.cpp

@@ -649,6 +649,18 @@ void CMasterActivity::done()

 void CMasterActivity::updateFileReadCostStats()
 {
+     // Returns updates numDiskReads & readCost in the file attributes and returns the readCost


should say
// Updates numDiskReads & readCost in the file attributes and returns the readCost

?

jakesmith

@shamser - looks good, please squash.

Signed-off-by: Shamser Ahmed <[email protected]>

jakesmith

@shamser - looks good.

jakesmith · 2023-12-06T12:55:01Z

dali/base/dadfs.cpp

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


trivial/typo; Dymically -> Dynamically

jakesmith · 2023-12-06T13:13:47Z

dali/ft/filecopy.cpp

@@ -3771,7 +3774,13 @@ void FileSprayer::updateTargetProperties()
    if (distributedSource)
    {
        if (distributedSource->querySuperFile()==nullptr)


per offline conversation - to be investigated.
What happens when 'distributedSource' is a superfile (is it even supported?). If it is, what happens (if anything currently) re. updating cost attributes on the subfiles read.
@shamser - I've opened a subtask for this (HPCC-30970), under HPCC-30965

shamser · 2023-12-06T14:22:41Z

RE: What happens when 'distributedSource' is a superfile (is it even supported?). If it is, what happens (if anything currently) re. updating cost attributes on the subfiles read.

Initial investigation shows that costs and other stats are not being updated when the distributedSource is a superfile. I created a ticket to work on this: https://track.hpccsystems.com/browse/HPCC-30973

shamser marked this pull request as ready for review November 24, 2023 14:51

shamser requested a review from jakesmith November 24, 2023 14:52

shamser force-pushed the issue30911 branch 3 times, most recently from 75bdb0a to d00eb36 Compare November 24, 2023 15:25

jakesmith requested changes Nov 27, 2023

View reviewed changes

shamser force-pushed the issue30911 branch from c13c618 to 561fbae Compare November 28, 2023 13:33

shamser requested a review from jakesmith November 28, 2023 13:34

jakesmith approved these changes Nov 28, 2023

View reviewed changes

shamser force-pushed the issue30911 branch from 561fbae to 9e3b9f4 Compare November 29, 2023 11:36

shamser requested a review from jakesmith November 29, 2023 11:42

jakesmith mentioned this pull request Nov 29, 2023

HPCC-30599 Fix file access costs for keyed join #18023

Merged

39 tasks

ghalliday reviewed Nov 29, 2023

View reviewed changes

jakesmith requested changes Nov 29, 2023

View reviewed changes

shamser force-pushed the issue30911 branch from 8716608 to b1b1b2f Compare November 30, 2023 12:19

shamser requested review from jakesmith and ghalliday November 30, 2023 12:21

ghalliday approved these changes Nov 30, 2023

View reviewed changes

jakesmith reviewed Nov 30, 2023

View reviewed changes

shamser force-pushed the issue30911 branch from 7c1d9fd to 0bed6f4 Compare November 30, 2023 16:20

shamser requested a review from jakesmith December 5, 2023 09:34

jakesmith reviewed Dec 5, 2023

View reviewed changes

shamser force-pushed the issue30911 branch from be8ec4b to 3cc32e5 Compare December 5, 2023 15:01

jakesmith approved these changes Dec 5, 2023

View reviewed changes

shamser force-pushed the issue30911 branch from 3cc32e5 to 6fef377 Compare December 6, 2023 09:05

HPCC-30911 Store read and write cost as file attributes

ca26282

Signed-off-by: Shamser Ahmed <[email protected]>

shamser force-pushed the issue30911 branch from 6fef377 to ca26282 Compare December 6, 2023 09:09

shamser requested a review from jakesmith December 6, 2023 09:11

jakesmith approved these changes Dec 6, 2023

View reviewed changes

ghalliday merged commit 1d01b18 into hpcc-systems:candidate-9.4.x Dec 7, 2023
47 checks passed

HPCC-30911 Store read and write cost as file attributes #18080

HPCC-30911 Store read and write cost as file attributes #18080

Conversation

shamser commented Nov 24, 2023 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Nov 24, 2023

jakesmith left a comment

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamser Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

ghalliday commented Nov 30, 2023

shamser commented Nov 30, 2023 • edited Loading

jakesmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamser commented Dec 6, 2023

shamser commented Nov 24, 2023 •

edited

Loading

shamser Nov 30, 2023 •

edited

Loading

shamser commented Nov 30, 2023 •

edited

Loading