SONATA compartment report performance improvements #239

hernando · 2018-08-22T13:41:09Z

No description provided.

karjonas · 2018-08-23T06:52:17Z

doc/Changelog.md

+  - Added two query options to configure chunking and chunk cache size when
+    opening/creating SONATA compartment reports.
+  - Added a new overload of brion::CompartmentReport::write to allow
+    optimizations for full frames writes.


full frame(s) writes.

karjonas · 2018-08-23T07:00:06Z

brion/plugin/compartmentReportHDF5.cpp

@@ -51,6 +51,11 @@ std::vector<hsize_t> _computeChunkDims(const std::vector<uint32_t>& cellSizes,
                                       float cellsToFramesRatio,
                                       size_t blockSize = _blockSize)
 {
+    if (cellsToFramesRatio == 0)


Write 0.f since this is float comparison?

Maybe also check for negative input.

I typically use integers and rely on the compiler to do the right thing. But it may be a bad practice on my side.
I've added the check in the parsing code, so this function will never get a negative number. Do you want an assert?

It is fine, you can leave it as is.

karjonas · 2018-08-23T07:09:16Z

brion/plugin/compartmentReportHDF5.cpp

@@ -89,6 +98,38 @@ bool _verifyFile(const HighFive::File& file)
    return true;
 }

+size_t _parseCacheSizeOption(const URI& uri)
+{
+    const auto i = uri.findQuery("cache_size");


What does i stand for?

It's an iterator to a std::pair<std::string, std::string> pair. I could call it keyValueIter.

I think it, iter or keyValueIter is fine.

karjonas · 2018-08-23T07:11:48Z

brion/plugin/compartmentReportHDF5.cpp

+        else
+            size = 0;
+    }
+    if (size == 0 and pos != value.size())


Should be enough to only check size == 0?

No because size == 0 is a valid input, it's used for disabling the cache (which may be beneficial in some situations)

karjonas · 2018-08-23T07:15:54Z

brion/plugin/compartmentReportHDF5.cpp

@@ -116,7 +158,9 @@ CompartmentReportHDF5::CompartmentReportHDF5(
            _parseBasicCellInfo();
            _subset = false;
        }
+        return;
    }


You can add an else clause and remove the return above.

I don't have a strong opinion here, but Stefan favored this coding style and it's used thorough all the library. If I do what you request, Daniel you suggest exactly the opposite.

Okay, lets keep it.

karjonas · 2018-08-23T08:02:14Z

brion/plugin/compartmentReportHDF5.cpp

+    // a full frame. Note that the maximum cache size allowed by this number
+    // and the actual maximum cache size are independent.
+    // To avoid collisions reading traces we also need a number of slots s,
+    // such as blocks[1] ∤ s. At the same time we want blocks[0] ∤ s.


Write "blocks[1] does not divide by s" to avoid using utf-8 symbols.

Is it a concern having utf-8 symbols in 2018?

I usually avoid it but I might be living in the past. Do as you please :)

It's a comment, so worst case it would look wrong.

tribal-tec · 2018-08-23T08:07:31Z

brion/plugin/compartmentReportHDF5.cpp

-           "  [file://]/path/to/report.(h5|hdf5)";
+           "  "
+           "[file://]/path/to/"
+           "report.(h5|hdf5)[?[cache_size=(auto|num_bytes):][cell_to_frames="


code uses cellS_to_frames

maybe try to put spaces differently to fix this horrendous clang formatting here?

It got formatted that way because I had a single string. If I split it by hand, clang-format won't break it.

tribal-tec · 2018-08-23T08:09:26Z

brion/plugin/compartmentReportHDF5.cpp

+
+    const auto& value = i->second;
+    if (value == "auto")
+        return std::numeric_limits<size_t>::max();


don't know too much about the intrinsics here, but this is more like 'max' cache size. Shall it not be depend on the size of the files or number of rows/frames/cells? 'auto' would imply something 'smart' to me.

It's something smart, but I found a bit annoying to document it in the description. Auto means allocating space for a whole frame or trace, whichever is bigger. It's not really max, std::numeric_limits<size_t>::max() is used as a magic value to identify this request in the client code. To make it more semantic I've replaced it with a constant.

tribal-tec · 2018-08-23T08:09:56Z

brion/plugin/compartmentReportHDF5.cpp

+                                       const double timestamp)
+{
+    if (gids.empty())
+        return true;


is more like 'false' to me intuitively

Not to me. If you ask me to read nothing, I will succeed reading nothing. Why should it be a failure?

tribal-tec · 2018-08-23T08:10:31Z

brion/plugin/compartmentReportHDF5.cpp

-                                      {frameCount, _targetMapping.frameSize});
-    slice.read(buffer);
-    return true;
+    // Cheking if the target GIDs are a single slice in the source file.


typo 'Cheking'

tribal-tec · 2018-08-23T08:11:08Z

brion/plugin/compartmentReportHDF5.cpp

+
+void CompartmentReportHDF5::_parseWriteOptions(const URI& uri)
+{
+    for (auto i = uri.queryBegin(); i != uri.queryEnd(); ++i)


const auto&

In this case it doesn't matter, but since this is just an iterator, don't you think that adding a reference would make the code slower for a long loop? Even with optimizations I have the feeling that the code would have more pointer chasing with the reference.

tribal-tec · 2018-08-23T08:11:40Z

brion/plugin/compartmentReportHDF5.cpp

+        const auto& value = i->second;
+        if (key == "cells_to_frames")
+        {
+            if (value == "inf")


it's not documented that 'inf' is valid value

Done, I've also documented what cache_size=auto does.

tribal-tec · 2018-08-23T08:12:33Z

brion/plugin/compartmentReportHDF5.cpp

+void CompartmentReportHDF5::_reopenDataSet(size_t cacheSizeHint)
+{
+    // Getting the chunk dims
+    auto properties = H5Dget_create_plist(_data->getId());


HighFive has no support for that?

No, because it's a getter and we have started adding support for different properties quite recently. For the few number of lines it is I prefer not messing with HighFive.

Chunk cache size can be configured with the query parameter cache_size Chunk dimensions can be modified to favor traces vs frames using the query parameter cells_to_frames.

hernando · 2018-08-24T12:51:16Z

Any more comments on this one?

karjonas · 2018-08-24T12:52:10Z

lgtm

hernando requested review from tribal-tec and karjonas August 22, 2018 13:41

karjonas reviewed Aug 23, 2018

View reviewed changes

tribal-tec reviewed Aug 23, 2018

View reviewed changes

Juan Hernando Vieites added 4 commits August 23, 2018 11:44

Remove unneeded HDF5 header for C++

4767419

Better support of chunking for writing/reading H5 compartment reports

fcab2fd

Chunk cache size can be configured with the query parameter cache_size Chunk dimensions can be modified to favor traces vs frames using the query parameter cells_to_frames.

New CompartmentReport::write method to allow full frame optimizations

c9e3e5d

Optimize partial H5 report reads when they map to a single slice.

7f85c79

hernando force-pushed the h5_report branch from 8cb1c18 to c460407 Compare August 23, 2018 10:06

Review comments

05319d7

hernando force-pushed the h5_report branch from c460407 to 05319d7 Compare August 23, 2018 11:42

tribal-tec approved these changes Aug 24, 2018

View reviewed changes

hernando merged commit de59dc1 into BlueBrain:master Aug 24, 2018

hernando deleted the h5_report branch August 27, 2018 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SONATA compartment report performance improvements #239

SONATA compartment report performance improvements #239

hernando commented Aug 22, 2018

karjonas Aug 23, 2018

karjonas Aug 23, 2018

karjonas Aug 23, 2018

hernando Aug 23, 2018

karjonas Aug 23, 2018

karjonas Aug 23, 2018

hernando Aug 23, 2018

karjonas Aug 23, 2018

karjonas Aug 23, 2018

hernando Aug 23, 2018

karjonas Aug 23, 2018

hernando Aug 23, 2018

karjonas Aug 23, 2018

karjonas Aug 23, 2018

hernando Aug 23, 2018

karjonas Aug 23, 2018

hernando Aug 23, 2018

tribal-tec Aug 23, 2018

tribal-tec Aug 23, 2018

hernando Aug 23, 2018

tribal-tec Aug 23, 2018

hernando Aug 23, 2018

tribal-tec Aug 23, 2018

hernando Aug 23, 2018

tribal-tec Aug 23, 2018

tribal-tec Aug 23, 2018

hernando Aug 23, 2018

tribal-tec Aug 23, 2018

hernando Aug 23, 2018

tribal-tec Aug 23, 2018

hernando Aug 23, 2018

hernando commented Aug 24, 2018

karjonas commented Aug 24, 2018

SONATA compartment report performance improvements #239

SONATA compartment report performance improvements #239

Conversation

hernando commented Aug 22, 2018

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

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

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

hernando commented Aug 24, 2018

karjonas commented Aug 24, 2018