HPCC-31649 New StSizePeakEphemeralDisk and StSizePeakTempDisk look ahead and hash distribute spilling #18585

shamser · 2024-04-25T15:26:27Z

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 · 2024-04-25T15:26:51Z

Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-31649

Jirabot Action Result:
Workflow Transition: Merge Pending
Updated PR

jakesmith

@shamser - looks good, but worth changing the linking semantics of CFileOwner (and e.g. leaving createSmartBuffer as it was).

And side-comments re. separate JIRA to track std. temp stats.

jakesmith · 2024-05-31T09:58:44Z

thorlcr/thorutil/thbuf.cpp

@@ -609,8 +607,7 @@ class CSmartRowInMemoryBuffer: public CSimpleInterface, implements ISmartRowBuff

 ISmartRowBuffer * createSmartBuffer(CActivityBase *activity, const char * tempname, size32_t buffsize, IThorRowInterfaces *rowif)
 {
-    Owned<IFile> file = createIFile(tempname);
-    return new CSmartRowBuffer(activity,file,buffsize,rowif);
+    return new CSmartRowBuffer(activity,createIFile(tempname),buffsize,rowif);


this is slightly at odds with the linking semantics of other utility classes, I think it better left as it was, where CSmartRowBuffer takes a link to the passed in IFile.

And, CFileOwner's iFile member is changed to a Linked, so it's consistent, and consitent with it's other Linked member). And then change the places that currently pass a linked file into CFileOwner to just pass the file and let it link.

Change to CFileOwner: #18726

ok, let's merge that one 1st, then change this back.

jakesmith · 2024-05-31T10:13:48Z

thorlcr/thorutil/thbuf.cpp

@@ -182,7 +182,8 @@ class CSmartRowBuffer: public CSimpleInterface, implements ISmartRowBuffer, impl
                size32_t left = nb*blocksize-mb.length();
                memset(mb.reserve(left),0,left);
            }
-            fileio->write(blk*(offset_t)blocksize,mb.length(),mb.bufferBase());
+            tempFileIO->write(blk*(offset_t)blocksize,mb.length(),mb.bufferBase());
+            tmpFileOwner.noteSize(numblocks*blocksize);


unrelated to this PR, but we're not capturing standard spill (temp) stats from this/these helper functions, i.e. StCycleSpillElapsedCycles, StTimeSpillElapsed, StSizeSpillFile ( NB: we do already in CThorRowCollectorBase )

Is there an existing JIRA? If not can you open one and link it to this/other JIRA's so it doesn't get lost?

Jira for this: https://hpccsystems.atlassian.net/browse/HPCC-31984

can you also link it to this JIRA and/or others (makes it easier to find in future)

jakesmith · 2024-05-31T10:19:02Z

thorlcr/thorutil/thbuf.cpp

@@ -1484,7 +1481,8 @@ class CSharedWriteAheadDisk : public CSharedWriteAheadBase
            mb.append((byte)0);
            size32_t len = mb.length();
            chunk.setown(getOutOffset(len)); // will find space for 'len', might be bigger if from free list
-            spillFileIO->write(chunk->offset, len, mb.toByteArray());
+            tempFileIO->write(chunk->offset, len, mb.toByteArray());
+            tempFileOwner->noteSize(highOffset);


as per comment above. If there isn't one, we need a JIRA to add the basic spill(temp) stats.

jira: https://hpccsystems.atlassian.net/browse/HPCC-31984

jakesmith · 2024-05-31T10:24:39Z

thorlcr/thorutil/thbuf.cpp

@@ -1536,7 +1533,8 @@ class CSharedWriteAheadDisk : public CSharedWriteAheadBase
        freeChunks.kill();
        freeChunksSized.kill();
        highOffset = 0;
-        spillFileIO->setSize(0);
+        tempFileIO->setSize(0);
+        tempFileOwner->noteSize(0);


I don't know why this (the setSize(0)) was implemented this way.
I think it should just delete the file, and let it be recreated if it was needed.

(not a change for now in this PR)

Jira to look into this later: https://hpccsystems.atlassian.net/browse/HPCC-31985

jakesmith · 2024-05-31T11:06:51Z

@shamser - infact this also covers a hashdistribute spilling, which is also used as part of hash dedup (so https://hpccsystems.atlassian.net/browse/HPCC-28757 alone won't give a full picture of a hash dedup;s spilling).

Can you update this JIRA and it's PR title to mention that it covers hash distribute spilling as well?

jakesmith

@shamser - looks good, but better to merge #18726 1st, then change the linking of the iFile back in createSmartBuffer before this is merged.

jakesmith · 2024-06-11T08:33:25Z

thorlcr/thorutil/thbuf.cpp

@@ -609,8 +607,7 @@ class CSmartRowInMemoryBuffer: public CSimpleInterface, implements ISmartRowBuff

 ISmartRowBuffer * createSmartBuffer(CActivityBase *activity, const char * tempname, size32_t buffsize, IThorRowInterfaces *rowif)
 {
-    Owned<IFile> file = createIFile(tempname);
-    return new CSmartRowBuffer(activity,file,buffsize,rowif);
+    return new CSmartRowBuffer(activity,createIFile(tempname),buffsize,rowif);


ok, let's merge that one 1st, then change this back.

jakesmith

@shamser - please see comments.

jakesmith · 2024-06-14T10:42:31Z

thorlcr/thorutil/thbuf.cpp

+        Owned<IFile> iFile = createIFile(spillName);
+        iFile->setShareMode(IFSHnone);
+        tempFileIO.setown(iFile->open(IFOcreaterw));
+        tempFileOwner.setown(new CFileOwner(iFile.getClear(), activity->queryTempFileSizeTracker()));


this now needs to be changed to avoid leaking iFile, e.g. to:

tempFileOwner.setown(new CFileOwner(iFile, activity->queryTempFileSizeTracker()));

jakesmith · 2024-06-14T10:44:58Z

thorlcr/thorutil/thbuf.cpp

-            fileio.clear();
-            file->remove();
-        }
+        if (tempFileIO)


trivial: no need to test

jakesmith · 2024-06-14T10:49:22Z

thorlcr/thorutil/thormisc.hpp

+                fileSizeTracker->growSize(size-fileSize);
+            else
+                fileSizeTracker->shrinkSize(fileSize-size);
+            fileSize = size;


If fileSizeTracker is not set, fileSize won't be updated. I think should be :

if (fileSize!=size) { if (fileSizeTracker) { if (size > fileSize) fileSizeTracker->growSize(size-fileSize); else fileSizeTracker->shrinkSize(fileSize-size); } fileSize = size; }

( Gavin made a similar comment in this PR : #18573 (comment) )

shamser · 2024-06-14T11:16:27Z

@shamser - please see comments.

Some changes made in #18573 will mean that this PR will need to be updated. I'll update this PR once 18573 is merged. @jakesmith

jakesmith

@shamser - looks good.

jakesmith · 2024-06-14T15:17:23Z

@shamser - there's a clash now, can you rebase

…k ahead and hash distribute spilling Signed-off-by: Shamser Ahmed <[email protected]>

shamser force-pushed the issue31649 branch 2 times, most recently from 7f3bc57 to 5a3bd59 Compare April 29, 2024 14:27

shamser changed the title ~~HPCC-31649 New StSizePeakEphemeralStorage and StSizePeakSubgraphTemp look ahead~~ HPCC-31649 New StSizePeakEphemeralDisk and StSizePeakTempDisk look ahead May 30, 2024

shamser force-pushed the issue31649 branch 2 times, most recently from 56580e5 to da22007 Compare May 30, 2024 12:32

shamser requested a review from jakesmith May 30, 2024 12:32

shamser marked this pull request as ready for review May 30, 2024 12:33

jakesmith reviewed May 31, 2024

View reviewed changes

shamser changed the title ~~HPCC-31649 New StSizePeakEphemeralDisk and StSizePeakTempDisk look ahead~~ HPCC-31649 New StSizePeakEphemeralDisk and StSizePeakTempDisk look ahead and hash distribute spilling Jun 3, 2024

shamser requested a review from jakesmith June 10, 2024 09:01

jakesmith reviewed Jun 11, 2024

View reviewed changes

shamser force-pushed the issue31649 branch 2 times, most recently from de5a065 to 84c3c04 Compare June 12, 2024 14:43

shamser requested a review from jakesmith June 12, 2024 14:43

jakesmith reviewed Jun 14, 2024

View reviewed changes

shamser force-pushed the issue31649 branch 2 times, most recently from 4d40d35 to 2353100 Compare June 14, 2024 14:43

shamser requested a review from jakesmith June 14, 2024 14:45

jakesmith approved these changes Jun 14, 2024

View reviewed changes

HPCC-31649 New StSizePeakEphemeralDisk and StSizePeakTempDisk for loo…

d4da9d4

…k ahead and hash distribute spilling Signed-off-by: Shamser Ahmed <[email protected]>

shamser force-pushed the issue31649 branch from 2353100 to d4da9d4 Compare June 14, 2024 15:25

ghalliday merged commit 786aa0d into hpcc-systems:candidate-9.6.x Jun 14, 2024
21 of 23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-31649 New StSizePeakEphemeralDisk and StSizePeakTempDisk look ahead and hash distribute spilling #18585

HPCC-31649 New StSizePeakEphemeralDisk and StSizePeakTempDisk look ahead and hash distribute spilling #18585

shamser commented Apr 25, 2024 •

edited

Loading

github-actions bot commented Apr 25, 2024

jakesmith left a comment

jakesmith May 31, 2024

shamser Jun 3, 2024

jakesmith Jun 11, 2024

jakesmith May 31, 2024

shamser Jun 3, 2024

jakesmith Jun 4, 2024

jakesmith May 31, 2024

shamser Jun 3, 2024

jakesmith May 31, 2024

shamser Jun 3, 2024

jakesmith commented May 31, 2024

jakesmith left a comment

jakesmith Jun 11, 2024

jakesmith left a comment

jakesmith Jun 14, 2024

jakesmith Jun 14, 2024

jakesmith Jun 14, 2024

shamser commented Jun 14, 2024 •

edited

Loading

jakesmith left a comment

jakesmith commented Jun 14, 2024

HPCC-31649 New StSizePeakEphemeralDisk and StSizePeakTempDisk look ahead and hash distribute spilling #18585

HPCC-31649 New StSizePeakEphemeralDisk and StSizePeakTempDisk look ahead and hash distribute spilling #18585

Conversation

shamser commented Apr 25, 2024 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Apr 25, 2024

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

jakesmith commented May 31, 2024

jakesmith left a 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

shamser commented Jun 14, 2024 • edited Loading

jakesmith left a comment

Choose a reason for hiding this comment

jakesmith commented Jun 14, 2024

shamser commented Apr 25, 2024 •

edited

Loading

shamser commented Jun 14, 2024 •

edited

Loading