HPCC-28555 Add blocked reader for unfiltered serial index reading #17551

jakesmith · 2023-07-04T15:09:23Z

Allow indexes to be read with a large granularity.
This is particularly important if reading from storage that
charges per read access.

Signed-off-by: Jake Smith [email protected]

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:

jakesmith · 2023-07-04T15:10:49Z

thorlcr/master/thmastermain.cpp

@@ -360,6 +360,7 @@ class CRegistryServer : public CSimpleInterface
        msg.append(THOR_VERSION_MAJOR).append(THOR_VERSION_MINOR);
        processGroup->serialize(msg);
        globals->serialize(msg);
+        getGlobalConfigSP()->serialize(msg);


this and related changes are so that workers have access to global config, and in particular the planes.

github-actions · 2023-07-04T15:21:12Z

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

richardkchapman

The commit title suggests this is for serial index reads, but my reading of the code is that it will be used for random index reads too.

richardkchapman · 2023-07-05T07:58:18Z

roxie/ccd/ccdfile.cpp

                            current.setown(f->open(IFOread));
+                            if (blockedIndexIOSize)


There doesn't appear to be any code to use the plane settings in Roxie?

that's true, mainly because I thought use case tends to be different in roxie, where unlikely to have some indexes on a fast/non-blob plane and others on a slow/blob plane.. - and therefore a per plane configuration setting would not be required as much.

richardkchapman · 2023-07-05T08:01:04Z

system/jlib/jfile.cpp

+}
+
+static constexpr size32_t defaultBlockedIndexIOKB = 1024;
+size32_t getBlockedIndexIOSize(const char *planeName, int configUseBlockedIndexIO, size32_t configBlockedIndeIOSize)


Spelling: configBlockedIndeIOSize should be configBlockedIndexIOSize

will correct

richardkchapman · 2023-07-05T08:02:05Z

system/jlib/jfile.cpp

+            blockedSize = configBlockedIndeIOSize;
+        if (0 == blockedSize)
+        {
+            // could cache, but not sure it's worth it


Feels like it might be worth it to cache, if Roxie ever called this - it's opening a lot of index files.

caching added.

richardkchapman · 2023-07-05T08:08:45Z

thorlcr/graph/thgraphslave.cpp

@@ -2046,9 +2047,15 @@ class CLazyFileIO : public CInterfaceOf<IFileIO>
        return iFileIO.getClear();
    }
 public:
-    CLazyFileIO(CFileCache &_cache, const char *_filename, const char *_id, IActivityReplicatedFile *_repFile, bool _compressed, IExpander *_expander, const StatisticsMapping & _statMapping=diskLocalStatistics)
-        : cache(_cache), filename(_filename), id(_id), repFile(_repFile), compressed(_compressed), expander(_expander), fileStats(_statMapping)
+    CLazyFileIO(CFileCache &_cache, const char *_filename, const char *_id, IActivityReplicatedFile *_repFile, bool _compressed, IExpander *_expander, const StatisticsMapping & _statMapping, size32_t _blockedFileIOSize)


This looks like it will potentially be used for non-index files too. If so, some names may be inappropriate. But not sure you really want to use for non-index files except possibly in FETCH activities, as you are probably already using large buffers anyway.

hadn't considered fetch case, but only use case (in hthor and thor) is where it is for index read, where it is expected to potentially be read serially.
name here (in CLazyFileIO) ctor and factory are "blockedFileIOSize", so not specifically for indexes, so I think naming is ok(?), albeit it may never be used for anything but indexes.

only used/passed through for [unfiltered] index reads at the moment.

richardkchapman · 2023-07-05T08:09:31Z

system/jlib/jfile.cpp

@@ -7643,3 +7643,110 @@ IAPICopyClient * createApiCopyClient(IStorageApiInfo * source, IStorageApiInfo *
    }
    return nullptr;
 }
+


I did not review this bit in detail as it looks like it's copied from my prior PR.

yep, your original commit is the 1st of the 2 commits in this PR, I made minor changes only to CBlockedFileIO

ghalliday

I'm not convinced this is quite right. There are two issues
i) The size that is read when reading something from an index
ii) The size of reads when reading unfiltered indexes

It is not clear what effect (i) will have on random reading. For branches it would be good, - but they tend to get loaded quickly and stay in memory. We would need some significant tests before were were convinced it was worthwhile. It may also have a significant memory impact if multiple parts are open at once.

Really (ii) is a special case of "what is the most efficient size to read from this storage". Possibly this should be generalised so there was a blockedReadSize on a plane which was used for unfiltered indexes and files.

I'll finish reviewing code and add any specific comments.

ghalliday · 2023-07-05T12:30:15Z

system/jlib/jfile.cpp

+            return io->read(pos, len, data);
+        if (!blockedFileIOBuffer)
+        {
+            blockedFileIOBuffer = malloc(blockSize);


This has problems if there are multiple CBlockedFileIO objects with different blockSizes. Currently it would potentially corrupt memory. If a thread local size was used instead you could prevent the corruption, but would the block size would be determined by whichever file was first read on that thread.
I think the idea may be fundamentally flawed for multithreaded access.

ghalliday

@jakesmith generally looks good. A few minor comments.

ghalliday · 2023-07-07T12:47:47Z

system/jlib/jptree.cpp

@@ -8878,7 +8878,7 @@ void executeConfigUpdaterCallbacks()
 {
    if (!configFileUpdater) // NB: executeConfigUpdaterCallbacks should always be called after configFileUpdater is initialized
        return;
-    configFileUpdater->executeCallbacks(componentConfiguration.getLink(), globalConfiguration.getLink());
+    configFileUpdater->executeCallbacks(componentConfiguration, globalConfiguration);


This looks like a separate fix for a leak. Should it be in a separate PR? (It depends if it also affects 8.12.x)

you're right, it is a separate leak fix, and it does effect 8.12.x.
I'll release a separate JIRA/PR: https://track.hpccsystems.com/browse/HPCC-29915

ghalliday · 2023-07-07T12:50:59Z

thorlcr/graph/thgraphslave.cpp

+        if (blockedFileIOSize) // enabled
+        {
+            if (compressed || expander)
+                throw makeStringExceptionV(0, "CLazyFileIO(%s): blockedFileIO cannot be used with compressed files", filename.get());


This should be ignored rather than throwing an error - in case we start using that default for all sequential file access.

will change.

@jakesmith hasn't been changed.

hm, I made the change somewhere, I'll redo.

ghalliday · 2023-07-07T13:08:56Z

system/jlib/jfile.cpp

@@ -7579,3 +7589,103 @@ IAPICopyClient * createApiCopyClient(IStorageApiInfo * source, IStorageApiInfo *
    }
    return nullptr;
 }
+
+
+class CBlockedFileIO : public CSimpleInterfaceOf<IFileIO>


Should have a comment that this is not thread safe, so can only be used from a single thread.

will add comment

ghalliday · 2023-07-07T13:08:59Z

system/jlib/jfile.cpp

+    }
+    virtual size32_t read(offset_t pos, size32_t len, void *data) override
+    {
+        if (len > blockSize || pos+len > fileSize || len==0)


Do you need the fileSize checking? It means that you may perform an extra stat call which will add latency if it is on blob storage. It should be possible to change the code so it copes with reading at the end of the file.
Also len==0 test is not needed.

agreed, refactored to avoid need for fileSize

jakesmith · 2023-07-10T10:47:24Z

@ghalliday - please review new commit changes.

ghalliday

@jakesmith one remaining change. Please squash after fixing so it is ready for merge.

ghalliday · 2023-07-11T09:55:14Z

thorlcr/graph/thgraphslave.cpp

+        if (blockedFileIOSize) // enabled
+        {
+            if (compressed || expander)
+                throw makeStringExceptionV(0, "CLazyFileIO(%s): blockedFileIO cannot be used with compressed files", filename.get());


@jakesmith hasn't been changed.

Allow indexes to be read with a large granularity. This is particularly important if reading from storage that charges per read access. Signed-off-by: Jake Smith <[email protected]>

jakesmith · 2023-07-11T10:44:02Z

@jakesmith one remaining change. Please squash after fixing so it is ready for merge.

addressed and squashed.
Please review/merge.

ghalliday · 2023-07-11T12:21:58Z

@jakesmith better not to also rebase at the same time as squashing - it makes it hard to spot the changes.

jakesmith commented Jul 4, 2023

View reviewed changes

jakesmith requested review from ghalliday and richardkchapman July 4, 2023 15:11

richardkchapman reviewed Jul 5, 2023

View reviewed changes

ghalliday requested changes Jul 5, 2023

View reviewed changes

ghalliday reviewed Jul 5, 2023

View reviewed changes

jakesmith changed the title ~~HPCC-28555 Add blocked IFileIO reader for serial index reading~~ HPCC-28555 Add blocked IFileIO reader for index reading Jul 6, 2023

jakesmith force-pushed the HPCC-28555-indexread-lookahead branch from 9d59f2b to 2f26e7d Compare July 6, 2023 12:06

jakesmith changed the title ~~HPCC-28555 Add blocked IFileIO reader for index reading~~ HPCC-28555 Add blocked reader for unfiltered serial index reading Jul 6, 2023

jakesmith requested a review from ghalliday July 6, 2023 13:03

jakesmith changed the base branch from candidate-9.2.x to candidate-9.0.x July 6, 2023 13:53

jakesmith force-pushed the HPCC-28555-indexread-lookahead branch 2 times, most recently from 9a00180 to 11d18c7 Compare July 6, 2023 14:53

ghalliday reviewed Jul 7, 2023

View reviewed changes

jakesmith force-pushed the HPCC-28555-indexread-lookahead branch from 0e866b8 to c652d32 Compare July 10, 2023 10:46

jakesmith requested a review from ghalliday July 10, 2023 10:47

ghalliday reviewed Jul 11, 2023

View reviewed changes

HPCC-28555 Add blocked reader for unfiltered serial index reading

87d12a6

Allow indexes to be read with a large granularity. This is particularly important if reading from storage that charges per read access. Signed-off-by: Jake Smith <[email protected]>

jakesmith force-pushed the HPCC-28555-indexread-lookahead branch from c652d32 to 87d12a6 Compare July 11, 2023 10:43

jakesmith requested review from ghalliday July 11, 2023 10:43

ghalliday approved these changes Jul 11, 2023

View reviewed changes

ghalliday merged commit 4a0099b into hpcc-systems:candidate-9.0.x Jul 11, 2023
50 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-28555 Add blocked reader for unfiltered serial index reading #17551

HPCC-28555 Add blocked reader for unfiltered serial index reading #17551

jakesmith commented Jul 4, 2023 •

edited

Loading

jakesmith Jul 4, 2023

github-actions bot commented Jul 4, 2023

richardkchapman left a comment

richardkchapman Jul 5, 2023

jakesmith Jul 5, 2023

jakesmith Jul 6, 2023

richardkchapman Jul 5, 2023

jakesmith Jul 5, 2023

richardkchapman Jul 5, 2023

jakesmith Jul 6, 2023

richardkchapman Jul 5, 2023

jakesmith Jul 5, 2023

jakesmith Jul 6, 2023

richardkchapman Jul 5, 2023

jakesmith Jul 5, 2023

ghalliday left a comment

ghalliday Jul 5, 2023

ghalliday left a comment

ghalliday Jul 7, 2023

jakesmith Jul 10, 2023

ghalliday Jul 7, 2023

jakesmith Jul 10, 2023

ghalliday Jul 11, 2023

jakesmith Jul 11, 2023

ghalliday Jul 7, 2023

jakesmith Jul 10, 2023

ghalliday Jul 7, 2023

jakesmith Jul 10, 2023

jakesmith commented Jul 10, 2023

ghalliday left a comment

ghalliday Jul 11, 2023

jakesmith commented Jul 11, 2023

ghalliday commented Jul 11, 2023

HPCC-28555 Add blocked reader for unfiltered serial index reading #17551

HPCC-28555 Add blocked reader for unfiltered serial index reading #17551

Conversation

jakesmith commented Jul 4, 2023 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

Choose a reason for hiding this comment

github-actions bot commented Jul 4, 2023

richardkchapman 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

ghalliday left a comment

Choose a reason for hiding this 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

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 Jul 10, 2023

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith commented Jul 11, 2023

ghalliday commented Jul 11, 2023

jakesmith commented Jul 4, 2023 •

edited

Loading