ADIOS2 User Side KVCache to accelerate the remote query access.

[Change72]

Structure

In KVCache v1.0, we use Redis (community version) as the local cache server, and hiredis as the local cache client for C++.

Build and Use

Users can follow the build example scripts (scripts/build_scripts/build-adios2-kvcache.sh) to turn on KVCache (by default is off). After the build, the user needs to start the remote server and cache server by 'source scripts/build_scripts/build-adios2-kvcache.sh --start'

Features

In KVCache v1.0, we support partially overlapped cases. We can utilize the existing cache to reduce network traffic even though the new request only partially overlaps with existing cached results. Users can also set the depth of overlap searching, by changing the environment variable: maxDepth (in source/adios2/engine/bp5/BP5Reader.cpp).

Redis provides auto-persistence, which means Redis will flush all the data to disk when it exits and re-load to memory when it restarts. Users can adjust Redis memory usage limit and other configurations in Redis configuration files.

[eisenhauer]

Before this can go through testing and be merged, your code has to be formatted according to our clang-format rules (which requires running the right version of clang-format). If you are on a machine with docker installed, the easiest way to accomplish this is to cd into your ADIOS2 directory and run "./scripts/developer/run-clang-format.sh". This will run docker and download an image with the appropriate version of clang-format. It'll generate a lot of "diff" output, but what's important for you is that it leaves the re-formatted files in place, so you can just do another git commit and all should be well. Note that the docker image is x86-based, but I've found that for this simple use even works on a Apple silicon if you're on a MacBook...

[vicentebolea]

need to add as at adios2_option in this file.

[Change72]

added

[vicentebolea]

Use CURL/wget rather than git here

[vicentebolea]

A future idea is that this could be a docker-compose file

[eisenhauer]

@Change72 If you click on "details" next to the "cdash" entry in the checks, then click on the yellow box indicating a failure in a build, you can see the warnings that resulted in that build failure... ADIOS doesn't allow warnings in CI, so these have to be corrected.

[eisenhauer]

Excellent work getting rid if the templating, and I'm thrilled that we don't have to base64 encode the data to store it in Redis. I'll do a more thorough review in a bit, but I think you've hit the big points I'd have.

[eisenhauer]

My lurking worry here is that we have expanded a simple loop with a 2 line body to 100+ lines. Maybe we can break out the KVCache case to a different subroutine to keep the non-cache case simple to understand? Also you might use CoreDims/DimsArray rather than Dims. It's probably not as critical here, but Dims uses std::vector, which means lots of tiny mallocs whenever these are created, destroyed, passed by value, etc. This caused serious performance problems in metadata manipulation with BP4 at scale. As a result, BP5 tries to avoid Dims.

[Change72]

1. I want to add a subroutine in:

   if (m_Remote)
   {
   PerformRemoteGets();
   }
   else
   {
   PerformLocalGets();
   }

However, what I can do is like:

#ifdef ADIOS2_HAVE_KVCACHE
if (getenv("useKVCache"))
{
PerformLocalGetsWithKVCache();
}
else
{
PerformRemoteGets();
}
#else
PerformRemoteGets();
#endif

do you think this is fine?

2. It's hard to initialize DimsArray when I declare ReqInfo struct, since I don't know the dimension of each Req, and DimsArray has no default construction method.

[anagainaru]

What about?

#ifdef ADIOS2_HAVE_KVCACHE
if (getenv("useKVCache"))
{
PerformLocalGetsWithKVCache();
}
else
#endif
PerformRemoteGets();

[eisenhauer]

I prefer this version...

[Change72]

Maybe I could get rid of getenv("useKVCache") ->

if (m_Remote)
{
  #ifdef ADIOS2_HAVE_KVCACHE
    PerformRemoteGetsWithKVCache();
  #else
    PerformRemoteGets();
  #endif
}
else
{
  PerformLocalGets();
}

If the user builds with KVCache and wants to use remote Gets (DoRemote=1), KVCache will be used automatically. However, if the user prefers to use only the remote service without KVCache, they will need to rebuild (possibly in a separate folder).

We could also consider checking whether the cache connection is active, and if it is not, default to using the pure remote service.

[eisenhauer]

2. It's hard to initialize DimsArray when I declare ReqInfo struct, since I don't know the dimension of each Req, and DimsArray has no default construction method.

So, let me tell you my worry. Your code in your cache stuff is in a lot of ways similar to the code in NdCopy (which looks at whether or not two blocks intersect, the extent of the overlap, etc.). We used to have some major performance problems in this when metadata was large (many thousand blocks per variable) because every time it did something like try to determine an intersection it ended up creating a destroying a dozen or more Dims vectors (doing things like converting a taking a StartEnd Box and creating a StartCount box from it, having all those temporary values get free'd when the intersection test returned, only to recreate them later, etc.). We traced the issue back to lots and lots of small malloc/free calls when those Dims were created, initialized and destroyed. It took a ton of effort to rewrite the worst parts of that code to use ArrayDims (stack allocated dimension array, so cheap to create/destroy, own their own data) or CoreDims (the equivalent of a "span", don't own their data but let us pass it in places and operate on it like a vector without a copy). With a lot of careful work kill all the object creation/destruction overhead in NdCopy so its only overhead was necessary control and data movement. (And if you go look at its source in helper/adiosMemory.cpp you'll see extensive use of ArrayDims and CoreDims.).

The storm of vector construction and destruction in ADIOS metadata handling wasn't noticeable at small scales and it was easy to ignore the implicit overhead of casual use of Dims, but as we ran at higher scales it became seriously problematic. You're probably not seeing an issue at the scales you're testing caching at either, but we hope to use this in large-metadata situations, so it'll become an issue if it's not done well. There's just too much similarity in your processing and the NdCopy case for me not to be concerned. We obviously didn't eliminate Dims from all the ADIOS code, but we killed it where it was used most intensively. You might do the same.

(And of course you do know the dimensionality of the variable in the place where you declare ReqInfo variable, it's in the Req variable that you're iterating over in that loop. Or it would be easy to add a default constructor and initialize the dimcount later. My concern is about places where there might be intensive vector usage.)

[Change72]

The test script we talked about just now is: https://github.com/Change72/adios-test/blob/master/TestCoreDims.cpp

// will make a value copy of dimsPoint2
DimsArray dimsPointer3(reinterpret_cast<CoreDims&>(dimsPointer2));

// will only copy the pointer
DimsArray dimsPointer3(dimsPointer2);

[Change72]

However, the following code will also give wrong results, but vector works fine.

helper::NdCopy(reinterpret_cast<char *>(ReqInfo.Data), reinterpret_cast<helper::CoreDims&>(ReqInfo.ReqBox.Start), reinterpret_cast<helper::CoreDims&>(ReqInfo.ReqBox.Count), true, false, reinterpret_cast<char *>(Req.Data), Req.Start, Req.Count, true, false, ReqInfo.TypeSize);

Vector code:

        std::vector<size_t> start;
        std::vector<size_t> count;
        ReqInfo.ReqBox.StartToVector(start);
        ReqInfo.ReqBox.CountToVector(count);
        helper::NdCopy(reinterpret_cast<char *>(ReqInfo.Data), start, count, true, false, reinterpret_cast<char *>(Req.Data), Req.Start, Req.Count, true, false, ReqInfo.TypeSize);

[eisenhauer]

At a quick page through, I don't have any complaints. I'll let Norbert look for final approval though.

[Change72]

@eisenhauer I just got a warning C4267: 'argument': conversion from 'size_t' to 'const int', possible loss of data

It happened in NdCopy const int typeSize

While in size_t GetDataTypeSize(DataType type), the return type is size_t

I just get size_t from GetDataTypeSize and then pass it to NdCopy, which causes the warning.

I am curious why typeSize is int rather than size_t in NdCopy? Could it be a negative value?

[eisenhauer]

The windows compilers are annoyingly picky about implicit type conversions. The typeSize argument to NdCopy is the size of the element. I.E. it'll be between 1 and the biggest type we support, complex double, which would be 16. The author of NdCopy chose to use an int for that, presumably not to waste a whole 8 bytes on something that doesn't need to be remotely that big. You can change your typeSize field in your Req to int, or you can add a cast into your NdCopy call. Or I guess if you were really ambitious you could change NdCopy to take size_t for that. But once you go down the rabbit hole of changing types, that stuff tends to snowball...

[eisenhauer]

The MacOS failure is expected at this point, but the sanitizer failure is weird. I've asked it to rerun to see if it was spurious.