Improve `mapped` and `head` modes. #21

greg7mdp · 2023-09-25T16:35:54Z

issue with heap mode: at leap startup and exit, we perform a memcpy copy between two mappings of the full database size. This causes significant pressure on the file cache when physical RAM is less than 2x the database size

__=> solution: use a series of smaller mappings for the copy, reducing RAM contention.
issue with mapped mode: when leap is running, linux will grind the disk into dust with its default dirty writeback algo. it's also a perf killer.

=> solution: map the file with MAP_PRIVATE (so changes are not written back to the file), and on exit copy just the modified pages using a series of RW mappings (info on which pages are dirty is gathered using the pagemap interface. Should we still set the dirty bit in the db file at startup?

This new implementation of mapped mode, as well as the existing heap and locked modes, does not allow sharing an opened database in RW mode with other instances in RO mode. In order to not completely remove the sharing functionality (tested in test.cpp), a new mapped_shared mode is introduced, which is the same as the old mapped mode.

Also:

update minimum C++ standard requirement to C++20
update ci/cd platform from debian:buster to debian:bullseye to get c++20 support (std::span)

Instead of two large file mappings, use a series of smapp mappings for the copy.

…earing the Soft-Dirty bits.

…d_heap_improvement

…e it is not available.

CMakeLists.txt

include/chainbase/pagemap_accessor.hpp

include/chainbase/pinnable_mapped_file.hpp

heifner

I didn't run any tests with this. Will lean on @spoonincode tests he is running.

…red` mode.

spoonincode · 2023-10-03T00:58:44Z

include/chainbase/pagemap_accessor.hpp

+#include <vector>
+#include <span>
+#include <boost/interprocess/managed_external_buffer.hpp>
+#include <boost/interprocess/anonymous_shared_memory.hpp>


This seems to be unused, and causes a compiler warning. Similarly for the same include file in pinnable_mapped_file.cpp

Thanks, I removed this and a couple other unneeded includes.

spoonincode · 2023-10-03T01:35:16Z

src/pinnable_mapped_file.cpp

@@ -216,7 +300,7 @@ void pinnable_mapped_file::setup_non_file_mapping() {
   round_up_mmaped_size(_2mb);
   _non_file_mapped_mapping = mmap(NULL, _non_file_mapped_mapping_size, PROT_READ|PROT_WRITE, common_map_opts, VM_FLAGS_SUPERPAGE_SIZE_2MB, 0);
   if(_non_file_mapped_mapping != MAP_FAILED) {
-      std::cerr << "CHAINBASE: Database \"" << _database_name << "\" using 2MB pages" << std::endl;
+      std::cerr << "CHAINBASE: Database \"" << _database_name << "\" using 2MB pages" << '\n';


what is the reasoning for changing all the std::endl to '\n'?

I get a warning from clang-tidy that it is a performance issue. I believe this is the reason.

But for log messages you typically do want the flush at the end of a message. Otherwise you have no guarantee how promptly the output will be visible to the user (or recorded in to some log ingest system). In the case of the % complete messages, without the flush none of these may be output until after 100% is already reached.

This can be seen with something like

#include <iostream> #include <thread> int main() { using namespace std::chrono_literals; std::cout << "Get Ready!" << std::endl; for(unsigned i = 0; i < 60; ++i) { std::cout << i << '\n'; std::this_thread::sleep_for(1s); } return 0; }

Then run something like

./o > output.txt & tail -f output.txt

and you will likely not see any output beyond Get Ready for an extended period of time (for me, until the application exits). Using endl instead shows each number counting up.

So for a user who sends stderr to a log file they won't see our % complete messages promptly.

cerr is non-buffered. Although, I would argue with sticking with std::endl as it is more readable and the flush should be a no-op for cerr.

I'd rather not add it back, unless you are worried it may cause a change of behavior. clang-tidy warns about it. And personally I don't find it more readable.

cerr is non-buffered

good point, I had forgot there was a difference there. So this change is really just ultimately a style change, and the clang-tidy commentary is not applicable

But for log messages you typically do want the flush at the end of a message.

OK, I'll add the flush back. Would you prefer I revert to what it was before, or I use std::cout << "Hello\n" << std::flush; as mentioned here.

But for log messages you typically do want the flush at the end of a message.

Like @heifner pointed out, since it's cerr it's not buffered by default anyways, so that comment from me isn't accurate in this case.

Personally, I'm fine either way (leaving as is or reverting or using flush or ..)

@spoonincode at this point I'm happy to make any change (or none) that are necessary for your approval today. Just let me know.

spoonincode · 2023-10-03T01:35:49Z

src/pinnable_mapped_file.cpp


      if(time(nullptr) != t) {
         t = time(nullptr);
-         std::cerr << "CHAINBASE: Preloading \"" << _database_name << "\" database file, " << offset/(_file_mapped_region.get_size()/100) << "% complete..." << std::endl;
+         std::cerr << "CHAINBASE: Preloading \"" << _database_name << "\" database file, " <<
+            offset/(_file_mapped_region.get_size()/100) << "% complete..." << '\n';


I'm getting a divide by zero on this line when running nodeos like

nodeos --delete-all --snapshot /home/spoon/RAMsnaps/snapshot-1287571bdd7b7337bcaac970f9cbc03b69fcd039a0784068b97152f130b483c5.bin --chain-state-db-size-mb 24576 --database-map-mode heap

…M becomes scarce.

src/pinnable_mapped_file.cpp

spoonincode · 2023-10-03T22:54:13Z

src/pinnable_mapped_file.cpp

+            offset += copy_size;
+         }
+      }
+   }


Won't this write out the same 1GB of dirty pages over and over again? I would have thought you need a call to clear_refs() to reset the bits back to clean. But.. the clear_refs interface is all-or-nothing: so you will need to write out all dirty pages.

yes, you are right, this doesn't help. I'll comment it out for now. I don't think the memory check I have is good enough to do a full dirty page write (I'm worried this might happen too often).

Fixed (kinda)

…rrectly as Matt pointed out.

spoonincode · 2023-10-04T20:42:26Z

src/pinnable_mapped_file.cpp

+   // non-sharable chainbase dbs using mapped mode are flushed to disk
+   // ----------------------------------------------------------------------------------
+   for (auto pmm : _instance_tracker)
+      pmm->save_database_file(true);


This mechanism doesn't look thread safe at all -- if someone ctors a chainbase on a thread it can cause corruption for any other chainbase in the process if they're busy being written to in a different thread.

I don't think we need to protect against that since leap never does that.

But since we still maintain this project a submodule for others to use, I wonder if we should somehow mention this limitation anywhere (in the header file or something?).. not sure

Good point, I'll add a mention.

spoonincode · 2023-10-04T20:53:35Z

src/pinnable_mapped_file.cpp

+            memcpy(dst, src+offset, copy_size);
+
+            if (flush) {
+               std::cerr << "CHAINBASE: Writing \"" << _database_name << "\" database file, flushing buffers..." << '\n';


I don't think this message makes sense to move in to the while loop. Previously it was only printed once: at the end. Now it's printed for every (non-zero) 1GB of memory -- it can even be printed more times than the status message if you're sinking more than 1GB/sec.

I think you can just get rid of this message entirely. Its original purpose was to indicate a final end stage that may not be accounted for in the percentage status messages. But if syncing every 1GB, that isn't needed any longer.

Yes, I agree.

spoonincode · 2023-10-04T21:21:47Z

src/pinnable_mapped_file.cpp

 }

 std::istream& operator>>(std::istream& in, pinnable_mapped_file::map_mode& runtime) {
   std::string s;
   in >> s;
   if (s == "mapped")
      runtime = pinnable_mapped_file::map_mode::mapped;
+   else if (s == "mapped_private")


Just looking around at other option names thinking about consistency.. prior to 5.0 I think the only option values with-/_ were eos-vm-jit, eos-vm-oc etc

It looks like 5.0's http-category-address now adds options like chain_ro,127.0.0.1:8080

sooo.. unless we want to back walk those to be chain-ro etc I guess mapped_private is fine

I'd rather not change that as I've already provided the writeup that was integrated into the release notes.

Hmm, that is unfortunate. Wish that would have been chain-ro.

spoonincode · 2023-10-05T02:29:23Z

CMakeLists.txt

 elseif(NOT CMAKE_CXX_STANDARD)
-   set(CMAKE_CXX_STANDARD 17)
+   set(CMAKE_CXX_STANDARD 20)


Not critical for this PR but something that can be done in the future,

This,

chainbase/CMakeLists.txt

Line 2 in 7817736

cmake_minimum_required( VERSION 3.5 )

should be bumped to 3.12 as that's the first version that knows c++20.

Also this entire if/elseif/endif block is logically nonsensical. My guess was it originally required c++11, and it would make sense in that case. I might suggest changing the way this is done to how the bls lib does it.

Will do in the next PR!

greg7mdp added 7 commits September 18, 2023 14:19

Incremental read/write for heap mode to reduce memory contention

e5cf68b

Instead of two large file mappings, use a series of smapp mappings for the copy.

Finish implementing the readonly mapped mode.

8213c33

In mapped mode, save only modified pages at exit.

c6735eb

Update cicd to use debian:bullseye instead of debian:buster.

93cd1d3

Avoid multiple calls to msync

19ee2e0

Use boost interprocess mmap APIs

235d956

Reuse _file_mapping instead of creating a new bip::file_mapping

66d3326

greg7mdp marked this pull request as draft September 26, 2023 21:02

greg7mdp added 10 commits September 26, 2023 18:00

Fix my previous change for flushing the region to disk.

832805a

Add instance tracker so that we can flush all dbs to disk before cl…

4cde714

…earing the Soft-Dirty bits.

Cleanup error cases.

4bc07e4

code cleanup and renaming some members.

619ba1f

Add missing Boost random dependency (needed in Leap).

4dcbb00

Update boost version

219e89b

Remove benchmark from default build.

6b4eda4

Reduce overlap of memory mappings existence.

6e1aa5a

Add description for clear_refs_failed error

d6c1dcc

Merge branch 'main' of github.com:AntelopeIO/chainbase into mapped_an…

4a8070e

…d_heap_improvement

greg7mdp force-pushed the mapped_and_heap_improvement branch from c63a9ed to 4a8070e Compare September 29, 2023 15:47

greg7mdp mentioned this pull request Sep 29, 2023

Improve chainbase mapped and heap behavior AntelopeIO/leap#1691

Merged

greg7mdp added 3 commits September 29, 2023 12:51

Remove unused code.

c3352cc

Add extra test mode mapped_shared.

d275422

Make sure we don't try to use the pagemap feature on platforms wher…

65eefd4

…e it is not available.

heifner requested changes Oct 2, 2023

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

include/chainbase/pagemap_accessor.hpp Show resolved Hide resolved

include/chainbase/pagemap_accessor.hpp Outdated Show resolved Hide resolved

include/chainbase/pinnable_mapped_file.hpp Show resolved Hide resolved

greg7mdp added 2 commits October 2, 2023 10:24

Remove leftover comment not necessary anymore.

abc648c

Address PR comments.

da2910c

greg7mdp marked this pull request as ready for review October 2, 2023 14:35

Add another commment.

7ae2b7c

heifner approved these changes Oct 2, 2023

View reviewed changes

Check for db file on tempfs and refuse to start unless in `mapped_sha…

e7a9b5a

…red` mode.

Add API to flush RW db and convert to RO mapping after snapshot.

6cce710

spoonincode reviewed Oct 3, 2023

View reviewed changes

spoonincode requested changes Oct 3, 2023

View reviewed changes

greg7mdp added 3 commits October 3, 2023 08:37

Fix divide by zero in heap mode.

4ced7af

Remove some unneeded includes.

44c9a20

mapped mode: add code to write some pages to disk when available RA…

4b7cf64

…M becomes scarce.

heifner reviewed Oct 3, 2023

View reviewed changes

src/pinnable_mapped_file.cpp Outdated Show resolved Hide resolved

heifner reviewed Oct 3, 2023

View reviewed changes

src/pinnable_mapped_file.cpp Outdated Show resolved Hide resolved

heifner reviewed Oct 3, 2023

View reviewed changes

src/pinnable_mapped_file.cpp Outdated Show resolved Hide resolved

greg7mdp added 2 commits October 3, 2023 15:04

Address PR comments.

4ab8944

Make new node the non-default one (mapped_private)

173287c

spoonincode reviewed Oct 3, 2023

View reviewed changes

Disable check_memory_and_flush_if_needed() which was not working co…

7ff3038

…rrectly as Matt pointed out.

spoonincode reviewed Oct 4, 2023

View reviewed changes

greg7mdp added 2 commits October 4, 2023 17:35

Address PR comment

d928ec5

Remove unneeded std::cerr message as per PR comment.

7817736

spoonincode approved these changes Oct 5, 2023

View reviewed changes

greg7mdp merged commit 3bfb0d0 into main Oct 5, 2023
2 checks passed

greg7mdp deleted the mapped_and_heap_improvement branch October 5, 2023 03:43

greg7mdp added a commit that referenced this pull request Oct 5, 2023

Bump required cmake version as suggested in PR #21.

91cf743

greg7mdp added a commit that referenced this pull request Oct 5, 2023

Cleanup compiler feature dependency as suggested in review of PR #21.

302274b

spoonincode mentioned this pull request Nov 6, 2023

New mapped_private mode: avoid crash by flushing dirty pages when memory pressure gets high #24

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `mapped` and `head` modes. #21

Improve `mapped` and `head` modes. #21

greg7mdp commented Sep 25, 2023 •

edited

Loading

heifner left a comment

spoonincode Oct 3, 2023

greg7mdp Oct 3, 2023

spoonincode Oct 3, 2023

greg7mdp Oct 3, 2023

spoonincode Oct 4, 2023

heifner Oct 4, 2023

greg7mdp Oct 4, 2023 •

edited

Loading

spoonincode Oct 4, 2023

greg7mdp Oct 4, 2023

spoonincode Oct 4, 2023

heifner Oct 4, 2023

greg7mdp Oct 4, 2023

spoonincode Oct 3, 2023 •

edited

Loading

greg7mdp Oct 3, 2023

spoonincode Oct 3, 2023

greg7mdp Oct 3, 2023

greg7mdp Oct 3, 2023

spoonincode Oct 4, 2023

greg7mdp Oct 4, 2023

greg7mdp Oct 4, 2023

spoonincode Oct 4, 2023

greg7mdp Oct 4, 2023

greg7mdp Oct 4, 2023

spoonincode Oct 4, 2023

greg7mdp Oct 4, 2023 •

edited

Loading

heifner Oct 5, 2023

spoonincode Oct 5, 2023

greg7mdp Oct 5, 2023 •

edited

Loading

Improve mapped and head modes. #21

Improve mapped and head modes. #21

Conversation

greg7mdp commented Sep 25, 2023 • edited Loading

heifner 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

greg7mdp Oct 4, 2023 • edited Loading

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

spoonincode Oct 3, 2023 • edited Loading

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

greg7mdp Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg7mdp Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

Improve `mapped` and `head` modes. #21

Improve `mapped` and `head` modes. #21

greg7mdp commented Sep 25, 2023 •

edited

Loading

greg7mdp Oct 4, 2023 •

edited

Loading

spoonincode Oct 3, 2023 •

edited

Loading

greg7mdp Oct 4, 2023 •

edited

Loading

greg7mdp Oct 5, 2023 •

edited

Loading