remove file mapping member in order to free file handles earlier (#401)

remove file_mapping member, see #400

Author: hendrikmuhs

keyvi/.clang-tidy

@@ -16,3 +16,11 @@ Checks: "*,
 "
 HeaderFilterRegex: ''
 FormatStyle:     none
+CheckOptions:
+  - key:             misc-include-cleaner.IgnoreHeaders
+    # The following files are ignored when clang-tidy analyzes header
+    # inclusions:
+    # - boost detail
+    value:           '
+                     boost/.*/detail/.*\.hpp;
+                     '

keyvi/include/keyvi/dictionary/fsa/automata.h

@@ -91,21 +91,23 @@ class Automata final {
   explicit Automata(const dictionary_properties_t& dictionary_properties, loading_strategy_types loading_strategy,
                     const bool load_value_store)
       : dictionary_properties_(dictionary_properties) {
-    file_mapping_ = boost::interprocess::file_mapping(dictionary_properties_->GetFileName().c_str(),
-                                                      boost::interprocess::read_only);
+    boost::interprocess::file_mapping file_mapping = boost::interprocess::file_mapping(
+        dictionary_properties_->GetFileName().c_str(), boost::interprocess::read_only);
 
     const boost::interprocess::map_options_t map_options =
         internal::MemoryMapFlags::FSAGetMemoryMapOptions(loading_strategy);
 
     TRACE("labels start offset: %d", dictionary_properties_->GetPersistenceOffset());
-    labels_region_ = boost::interprocess::mapped_region(file_mapping_, boost::interprocess::read_only,
-                                                        dictionary_properties_->GetPersistenceOffset(),
-                                                        dictionary_properties_->GetSparseArraySize(), 0, map_options);
+    labels_region_ = boost::interprocess::mapped_region(
+        file_mapping, boost::interprocess::read_only,
+        static_cast<boost::interprocess::offset_t>(dictionary_properties_->GetPersistenceOffset()),
+        dictionary_properties_->GetSparseArraySize(), nullptr, map_options);
 
     TRACE("transitions start offset: %d", dictionary_properties_->GetTransitionsOffset());
     transitions_region_ = boost::interprocess::mapped_region(
-        file_mapping_, boost::interprocess::read_only, dictionary_properties_->GetTransitionsOffset(),
-        dictionary_properties_->GetTransitionsSize(), 0, map_options);
+        file_mapping, boost::interprocess::read_only,
+        static_cast<boost::interprocess::offset_t>(dictionary_properties_->GetTransitionsOffset()),
+        dictionary_properties_->GetTransitionsSize(), nullptr, map_options);
 
     const auto advise = internal::MemoryMapFlags::FSAGetMemoryMapAdvices(loading_strategy);
 
@@ -117,7 +119,7 @@ class Automata final {
 
     if (load_value_store) {
       value_store_reader_.reset(
-          internal::ValueStoreFactory::MakeReader(dictionary_properties_->GetValueStoreType(), &file_mapping_,
+          internal::ValueStoreFactory::MakeReader(dictionary_properties_->GetValueStoreType(), &file_mapping,
                                                   dictionary_properties_->GetValueStoreProperties(), loading_strategy));
     }
   }
@@ -415,7 +417,6 @@ class Automata final {
  private:
   dictionary_properties_t dictionary_properties_;
   std::unique_ptr<internal::IValueStoreReader> value_store_reader_;
-  boost::interprocess::file_mapping file_mapping_;
   boost::interprocess::mapped_region labels_region_;
   boost::interprocess::mapped_region transitions_region_;
   unsigned char* labels_;

keyvi/include/keyvi/dictionary/fsa/internal/memory_map_manager.h

@@ -26,8 +26,10 @@
 #define KEYVI_DICTIONARY_FSA_INTERNAL_MEMORY_MAP_MANAGER_H_
 
 #include <algorithm>
+#include <cstring>
 #include <fstream>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include <boost/filesystem.hpp>
@@ -59,13 +61,7 @@ class MemoryMapManager final {
   MemoryMapManager(const size_t chunk_size, const boost::filesystem::path directory,
                    const boost::filesystem::path filename_pattern)
       : chunk_size_(chunk_size), directory_(directory), filename_pattern_(filename_pattern) {}
-
-  ~MemoryMapManager() {
-    for (auto& m : mappings_) {
-      delete m.mapping_;
-      delete m.region_;
-    }
-  }
+  ~MemoryMapManager() = default;
 
   /* Using GetAdress to read multiple bytes is unsafe as it might be a buffer overflow
    *
@@ -82,7 +78,8 @@ class MemoryMapManager final {
 
     void* chunk_address = GetChunk(chunk_number);
 
-    return (reinterpret_cast<char*>(chunk_address) + chunk_offset);
+    // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic)
+    return static_cast<char*>(chunk_address) + chunk_offset;
   }
 
   /* Get a buffer as copy.
@@ -97,12 +94,15 @@ class MemoryMapManager final {
     size_t second_chunk_size = buffer_length - first_chunk_size;
 
     void* chunk_address = GetChunk(chunk_number);
-    std::memcpy(buffer, reinterpret_cast<char*>(chunk_address) + chunk_offset, first_chunk_size);
+
+    // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic)
+    std::memcpy(buffer, static_cast<char*>(chunk_address) + chunk_offset, first_chunk_size);
 
     if (second_chunk_size > 0) {
       void* chunk_address_part2 = GetChunk(chunk_number + 1);
-      std::memcpy(reinterpret_cast<char*>(buffer) + first_chunk_size,
-                  reinterpret_cast<const char*>(chunk_address_part2), second_chunk_size);
+      // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic)
+      std::memcpy(static_cast<char*>(buffer) + first_chunk_size, static_cast<const char*>(chunk_address_part2),
+                  second_chunk_size);
     }
   }
 
@@ -129,8 +129,9 @@ class MemoryMapManager final {
       size_t copy_size = std::min(remaining, chunk_size_ - chunk_offset);
       TRACE("copy size: %ld", copy_size);
 
-      std::memcpy(reinterpret_cast<char*>(chunk_address) + chunk_offset,
-                  reinterpret_cast<const char*>(buffer) + buffer_offset, copy_size);
+      // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic)
+      std::memcpy(static_cast<char*>(chunk_address) + chunk_offset, static_cast<const char*>(buffer) + buffer_offset,
+                  copy_size);
 
       remaining -= copy_size;
       tail_ += copy_size;
@@ -154,7 +155,8 @@ class MemoryMapManager final {
     void* chunk_address = GetChunk(chunk_number);
     size_t first_chunk_size = std::min(buffer_length, chunk_size_ - chunk_offset);
 
-    if (std::memcmp(reinterpret_cast<char*>(chunk_address) + chunk_offset, buffer, first_chunk_size) != 0) {
+    // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic)
+    if (std::memcmp(static_cast<char*>(chunk_address) + chunk_offset, buffer, first_chunk_size) != 0) {
       return false;
     }
 
@@ -165,9 +167,10 @@ class MemoryMapManager final {
 
     // handle overflow
     void* chunk_address_part2 = GetChunk(chunk_number + 1);
-    return (std::memcmp(reinterpret_cast<const char*>(chunk_address_part2),
-                        reinterpret_cast<const char*>(buffer) + first_chunk_size,
-                        buffer_length - first_chunk_size) == 0);
+
+    return (std::memcmp(static_cast<const char*>(chunk_address_part2),
+                        // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic)
+                        static_cast<const char*>(buffer) + first_chunk_size, buffer_length - first_chunk_size) == 0);
   }
 
   void Write(std::ostream& stream, const size_t end) const {
@@ -195,7 +198,7 @@ class MemoryMapManager final {
         size_t bytes_in_chunk = std::min(chunk_size_, remaining);
         TRACE("write chunk %d, with size: %ld, remaining: %ld", chunk, bytes_in_chunk, remaining);
 
-        const char* ptr = reinterpret_cast<const char*>(mappings_[chunk].region_->get_address());
+        const char* ptr = static_cast<const char*>(mappings_[chunk].get_address());
         stream.write(ptr, bytes_in_chunk);
 
         remaining -= bytes_in_chunk;
@@ -212,9 +215,7 @@ class MemoryMapManager final {
   void Persist() {
     persisted_ = true;
     for (auto& m : mappings_) {
-      m.region_->flush();
-      delete m.region_;
-      delete m.mapping_;
+      m.flush();
     }
 
     // truncate last file according to the written buffers
@@ -236,13 +237,8 @@ class MemoryMapManager final {
   size_t GetNumberOfChunks() const { return number_of_chunks_; }
 
  private:
-  struct mapping {
-    boost::interprocess::file_mapping* mapping_;
-    boost::interprocess::mapped_region* region_;
-  };
-
   size_t chunk_size_;
-  std::vector<mapping> mappings_;
+  std::vector<boost::interprocess::mapped_region> mappings_;
   boost::filesystem::path directory_;
   boost::filesystem::path filename_pattern_;
   size_t tail_ = 0;
@@ -262,13 +258,11 @@ class MemoryMapManager final {
       CreateMapping();
     }
 
-    return mappings_[chunk_number].region_->get_address();
+    return mappings_[chunk_number].get_address();
   }
 
   void CreateMapping() {
     TRACE("create new mapping %d", number_of_chunks_ + 1);
-    mapping new_mapping;
-
     boost::filesystem::path filename = GetFilenameForChunk(number_of_chunks_);
 
     std::ofstream chunk(filename.string().c_str(),
@@ -287,16 +281,15 @@ class MemoryMapManager final {
       throw memory_map_manager_exception("failed to create chunk (setting size)");
     }
 
-    new_mapping.mapping_ =
-        new boost::interprocess::file_mapping(filename.string().c_str(), boost::interprocess::read_write);
+    boost::interprocess::file_mapping const mapping(filename.string().c_str(), boost::interprocess::read_write);
 
-    new_mapping.region_ =
-        new boost::interprocess::mapped_region(*new_mapping.mapping_, boost::interprocess::read_write);
+    boost::interprocess::mapped_region mapped_region(
+        boost::interprocess::mapped_region(mapping, boost::interprocess::read_write));
 
     // prevent pre-fetching pages by the OS which does not make sense as values usually fit into few pages
-    new_mapping.region_->advise(boost::interprocess::mapped_region::advice_types::advice_random);
+    mapped_region.advise(boost::interprocess::mapped_region::advice_types::advice_random);
 
-    mappings_.push_back(new_mapping);
+    mappings_.push_back(std::move(mapped_region));
     ++number_of_chunks_;
   }
 };