Capture ability of BufferedReader to provide contiguous min len buffers as a trait

[kskalski]

Problem

As part of a larger change to provide io_uring replacement for BufferedReader I want to capture its functionality and API as a well-defined trait(s).
Some uses of BufferedReader are also mixed-up with extra reads done through read_into_buffer to overflow buffers - that could be better encapsulated in BufferedReader impl and trait.

Summary of Changes

This PR adds ContiguousBufFileRead that addresses the functionalities of:

• reading buffer with default requirement for returned buffer size -> new trait function fill_buf_required
• reading buffer with required size potentially exceeding internal buffer, with fallback to overflow buffer -> new trait function fill_buf_required_or_overflow
• getting underlying file offset -> trait function get_file_offset()

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 98.07692% with 4 lines in your changes missing coverage. Please review.

Project coverage is 83.2%. Comparing base (7db66d5) to head (e75d483).
Report is 15 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #6921   +/-   ##
=======================================
  Coverage    83.2%    83.2%           
=======================================
  Files         853      853           
  Lines      375116   375172   +56     
=======================================
+ Hits       312208   312273   +65     
+ Misses      62908    62899    -9     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[kskalski]

One note here: maybe we should also capture this code https://github.com/anza-xyz/agave/pull/6921/files#diff-47c90ea28df1e8cf4668e31f3096d7cd870014a5ba791767bdd789e047a63515R1089-R1110 into a dedicated trait function, e.g. something like fill_buf_with_overflow(&mut self, overflow_fallback: &mut Vec<u8>) -> &[u8]

[kskalski]

I changed the approach - the new trait functions take required data len as parameter and there is also some code refactoring to make accounts scan only use the trait instead of directly reading from file on some occasions.

[kskalski]

Reduced the diff a bit more and I think this is fine to review now.

Next step will be to share buffered reader (as &mut impl ContiguousBufFileRead I suppose) across scan functions and move overflow buffer into BufferedReader state (at the same time or in subsequent PR).

[brooksprumo]

Reduced the diff a bit more and I think this is fine to review now.

👀 ok will take a look

Next step will be to share buffered reader (as &mut impl ContiguousBufFileRead I suppose) across scan functions and move overflow buffer into BufferedReader state (at the same time or in subsequent PR).

Do we (and then why do we) want to actually share the buffered reader?

[kskalski]

I think it will be beneficial to share buffered reader even just to get rid of the overflow buffer re-allocation for each storage - this buffer can be allocated once per thread and be reused for the whole scan.

Ideally the shared state can be captured as a single trait (will need to be a bit different than one in this PR - the overflow fn should be hidden as internal detail) - this way we can replace implementation to io_uring reader without even touching all those scan functions / files.

[brooksprumo]

Ok, made a first pass. Will do another.

[brooksprumo]

[brooksprumo]

IMO the names for both of these functions is kinda weird. I don't have a good recommendation though, so I'm fine with leaving them as-is for now. We can always rename later if/when we have a better alternative.

[kskalski]

There were some bugs in the way capacity of overflow buffer was managed - it's fixed now and I tested it generating index with and without secondary index (thus triggering the two ways buffered reader is used).

IMO the names for both of these functions is kinda weird. I don't have a good recommendation though, so I'm fine with leaving them as-is for now. We can always rename later if/when we have a better alternative

Yeah, there is going to be some refinement in which methods we will need, in the next PR overflow buffer will be encapsulated into a wrapper struct and fill_buf_required_or_overflow will be removed in favor of only using fill_buf_required.

[brooksprumo]

There were some bugs in the way capacity of overflow buffer was managed - it's fixed now and I tested it generating index with and without secondary index (thus triggering the two ways buffered reader is used).

Do we have testing gaps then?

[kskalski]

Do we have testing gaps then?

Good question that I was also discussing with alessandro - it seems CI tests don't exercise large accounts-db data (in terms of bytes and number of variable sizes files). The bug here was due to unsafe code and memory corruption, but I agree it should be triggered in automatic tests.

Separate question is about adding command to CLI tool that will make processing snapshots and acccounts-db possible to do separate from running validator.

[brooksprumo]

Good question that I was also discussing with alessandro - it seems CI tests don't exercise large accounts-db data (in terms of bytes and number of variable sizes files). The bug here was due to unsafe code and memory corruption, but I agree it should be triggered in automatic tests.

I see. Yeah, if it requires large accounts-db, that'll be harder to do in a unit test.

Separate question is about adding command to CLI tool that will make processing snapshots and acccounts-db possible to do separate from running validator.

agave-ledger-tool processes snapshots. Does that work? Probably agave-ledger-tool verify is what to look at first.

[cpubot]

Do we have testing gaps then?

Good question that I was also discussing with alessandro - it seems CI tests don't exercise large accounts-db data (in terms of bytes and number of variable sizes files). The bug here was due to unsafe code and memory corruption, but I agree it should be triggered in automatic tests.

We should have at least one test here that exercises the overflow buffer via a max size data account and variable sized files:

agave/accounts-db/src/append_vec.rs

Lines 1684 to 1728 in b4ba976

	fn rand_exhaustive_append_vec(
	num_accounts: usize,
	) -> (
	ManuallyDrop<AppendVec>,
	Vec<(Pubkey, AccountSharedData)>,
	TempFile,
	) {
	let mut rng = thread_rng();
	let mut create_account = |data_len: usize| -> (Pubkey, AccountSharedData) {
	let pubkey = Pubkey::new_from_array(rng.gen());
	let owner = Pubkey::new_from_array(rng.gen());
	let mut account = AccountSharedData::new(rng.gen(), data_len, &owner);
	// Ensure we actually have some unique data to compare against when checking correctness
	let data = std::iter::from_fn(|| Some(rng.gen::<u8>()))
	.take(data_len)
	.collect::<Vec<_>>();
	account.set_data(data);
	(pubkey, account)
	};
	let mut test_accounts = Vec::with_capacity(num_accounts);
	let mut file_size = 0;
	for i in 0..num_accounts {
	let data_len = match i {
	// ensure one max size account
	0 => MAX_PERMITTED_DATA_LENGTH as usize,
	// ensure one 64KiB account
	x if x == num_accounts - 1 => 1 << 16,
	// Otherwise use a reasonably small account to avoid long test times
	x => x % 256,
	};
	let account = create_account(data_len);
	let size = aligned_stored_size(account.1.data().len());
	file_size += size;
	test_accounts.push(account);
	}
	let path = get_append_vec_path("test_scan_accounts_stored_meta_correctness");
	let av = ManuallyDrop::new(AppendVec::new(&path.path, true, file_size));
	let slot = 42;
	av.append_accounts(&(slot, test_accounts.as_slice()), 0)
	.unwrap();
	av.flush().unwrap();
	(av, test_accounts, path)
	}

Was the bug behavior you encountered not triggered by this? Is the bug currently present in master, or something encountered with the rewrite? What was the nature of the bug?

[kskalski]

The nature of the bug is that reserve or reserve_exact to set capacity is relative to the current len(), not to the current capacity, so using reserve_exact(new_capacity-old_capacity) is incorrect and instead it should be reserve_exact(new_capacity-vec.len()). Since the code then set len to the new_capacity with unsafe method, the actual underlying memory allocated might be shorter than len.

Hm... actually, this bug exists already in the old code on master, not sure why it wasn't causing trouble so far, but basically it is a silent memory corruption.

[kskalski]

Looking closer at master's code, it is a bit misleading as it calculates additional reserve using capacity, it actually keeps the invariant that len follows capacity 1:1, while I had this condition broken.

[kskalski]

We should have at least one test here that exercises the overflow buffer via a max size data account and variable sized files:

updated rand_exhaustive_append_vec( to generate several accounts with increasing sizes from 0 to max and confirmed that this triggers memory corruption with the bug I had earlier.

[brooksprumo]

Please wait for approval from @cpubot before merging.

[cpubot]

LGTM 👍