file size limit? by cheme · Pull Request #237 · paritytech/parity-db

cheme · 2024-04-04T14:59:46Z

Been ask a few time for a size limit on parity db files.

In itself it does not really make sense, but practically I believe that many tools are pretty bad at using big files (like not even chunking them).
Still this should not be default, but can be of use.

So I try to do a quick implementation to see if it can be a good idea to have.

Got this branch that seems to run the stress test: option max_file_size is in megabyte.

A few todo to handle and test to modify (so we have some runing with multiple files) and this could be good to go (an maybe use smallvec instead of vec).

cc\ @arkpar @MattHalpinParity @PierreBesson

BulatSaif · 2024-04-04T15:27:00Z

Is this change backward compatible? For example, if I create a snapshot for a node with the max_file_size parameter, and someone downloads it and starts the node without specifying max_file_size.

cheme · 2024-04-04T15:30:28Z

Is this change backward compatible? For example, if I create a snapshot for a node with the max_file_size parameter, and someone downloads it and starts the node without specifying max_file_size.

the parameter cannot be change, it is written in metadata, so should error when trying to open with a wrong parameter.
so depends on how it will be integrated with substrate, but there is no switching this max size.

arkpar · 2024-04-05T08:47:59Z

Great stuff.
Ideally we could map files to a sequeantial address space, so that looking up mapping addres based on chunk/entry won't be required. But the benefits probably will be negligible.

One thing I'm concerned about is hitting the OS limit for memory maps (/proc/sys/vm/max_map_count). We should run polkadot archive DB with this and see how many mmaps it creates and how close we are to the limit.

cheme · 2024-04-05T08:52:47Z

src/file.rs

 	const RESERVE_ADDRESS_SPACE: usize = 64 * 1024 * 1024; // 64 Mb

-	let map_len = len + RESERVE_ADDRESS_SPACE;
+	let map_len = len + RESERVE_ADDRESS_SPACE; // TODO should be max??


Wondering it it should be: std::cmp::max(RESERVE_ADDRESS_SPACE, len)

RESERVE_ADDRESS_SPACE is supposed to be additional reserved space. I.e. if RESERVE_ADDRESS_SPACE is 1024Mb and we try to map 1025Mb it should reserve additional space up to 2048Mb so that when next time we try to map 1026Mb it could use the same space.

if max_len is set, it should be used as max map size instead of RESERVE_ADDRESS_SPACE

cheme · 2024-04-05T08:53:46Z

src/file.rs

-				let file = self.create_file()?;
-				let len = GROW_SIZE_BYTES;
-				try_io!(file.set_len(len));
-				let map = mmap(&file, 0)?;


Here I switched from mmap(&file, 0) to mmap(&file, len).

cheme · 2024-04-05T10:30:43Z

One thing I'm concerned about is hitting the OS limit for memory maps (/proc/sys/vm/max_map_count). We should run polkadot archive DB with this and see how many mmaps it creates and how close we are to the limit.

yes, sure is convenient being able to define 1mb files (for testing), but I would not really expect something lower than 512Mb as max size, still with a db of 200gb, that would be more than 400 open mmap: clearly something to test.

MattHalpinParity · 2024-04-09T00:45:34Z

src/index.rs

-		madvise_random(&mut map);
-		log::debug!(target: "parity-db", "Opened existing index {}", id);
-		Ok(Some(IndexTable { id, path, map: RwLock::new(Some(map)) }))
+			try_io!(file.set_len(file_size(id.index_bits())));


Looks like all the index files are getting the original total size rather than smaller size based on max_chunks.

great catch, thanks 👍

MattHalpinParity · 2024-04-09T00:48:32Z

src/index.rs

+					.create_new(true)
+					.open(path.as_path()));
+				log::debug!(target: "parity-db", "Created new index {}", self.id);
+				try_io!(file.set_len(file_size(self.id.index_bits())));


Same file size issue as above.

MattHalpinParity · 2024-04-09T00:52:12Z

src/ref_count.rs

+				Ok(file) => file,
+			};
+
+			try_io!(file.set_len(file_size(id.index_bits())));


Same as with index.

MattHalpinParity · 2024-04-09T00:53:17Z

src/ref_count.rs

+					.create_new(true)
+					.open(path.as_path()));
+				log::debug!(target: "parity-db", "Created new ref count {}", self.id);
+				try_io!(file.set_len(file_size(self.id.index_bits())));


arkpar · 2024-04-09T20:59:30Z

src/file.rs

 pub struct TableFile {
-	pub map: RwLock<Option<(memmap2::MmapMut, std::fs::File)>>,
-	pub path: std::path::PathBuf,
+	pub map: RwLock<Vec<(memmap2::MmapMut, std::fs::File)>>,


should be called maps or mem_maps

arkpar · 2024-04-09T21:02:36Z

src/file.rs

 			},
 		};
-		let capacity = new_len / entry_size as u64;
+		let per_page_capacity = self.max_size.map(|m| m / entry_size as usize).unwrap_or(0);


entries_per_file. Let's not introduce terms such as "page" here.

arkpar · 2024-04-09T21:04:08Z

src/file.rs

-					let new_map = mmap(&file, new_len as usize)?;
-					let old_map = std::mem::replace(map, new_map);
-					try_io!(old_map.flush());
+		let map_and_file_len = map_and_file.len();


arkpar · 2024-04-09T21:04:53Z

src/file.rs

@@ -180,44 +217,72 @@ impl TableFile {

 	pub fn grow(&self, entry_size: u16) -> Result<()> {
 		let mut map_and_file = self.map.write();


src/ref_count.rs

MattHalpinParity · 2024-05-01T11:28:53Z

src/file.rs

+					capacity += GROW_SIZE_BYTES / entry_size as u64;
+					try_io!(file.set_len(GROW_SIZE_BYTES));
+				} else {
+					capacity = len / entry_size as u64;


Think this should be +=. Though it doesn't cause any obvious problems as if the value is incorrect then grow is called and the correct capacity is calculated there.

Thanks, sure += is right here.

…iles

cheme added 2 commits April 4, 2024 16:52

initial impl (some todos)

99e7a3e

loom

6feb91e

cheme commented Apr 5, 2024

View reviewed changes

MattHalpinParity reviewed Apr 9, 2024

View reviewed changes

src/ref_count.rs Outdated

Ok(file) => file,

};

try_io!(file.set_len(file_size(id.index_bits())));

Copy link

Contributor

MattHalpinParity Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as with index.

MattHalpinParity reviewed Apr 9, 2024

View reviewed changes

arkpar reviewed Apr 9, 2024

View reviewed changes

cheme added 7 commits April 15, 2024 10:00

fix index file size

f6b8a8f

copy change

f4033b8

remove useless field

51ee857

renamings

0ab21e9

limit reserve

24ba453

flush range on first only

25b1c1b

loom

ab870fb

MattHalpinParity reviewed Apr 26, 2024

View reviewed changes

src/ref_count.rs Outdated Show resolved Hide resolved

Update src/ref_count.rs

75080c0

MattHalpinParity reviewed May 1, 2024

View reviewed changes

cheme added 2 commits May 2, 2024 09:28

fix on initial capacity (from review)

42bb723

Merge branch 'split_files' of github.com:cheme/parity-db into split_f…

e286952

…iles

		@@ -180,44 +217,72 @@ impl TableFile {

		pub fn grow(&self, entry_size: u16) -> Result<()> {
		let mut map_and_file = self.map.write();

Conversation

cheme commented Apr 4, 2024

Uh oh!

BulatSaif commented Apr 4, 2024

Uh oh!

cheme commented Apr 4, 2024

Uh oh!

arkpar commented Apr 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cheme commented Apr 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants