From 42b6033f3c367ccce37c82356183d165d37ae881 Mon Sep 17 00:00:00 2001 From: JP Sugarbroad Date: Sat, 8 May 2021 01:18:03 -0700 Subject: [PATCH 1/3] Remove almost all unsafe code from Tree. --- git-odb/src/pack/index/traverse/indexed.rs | 4 +- git-odb/src/pack/index/write/encode.rs | 6 +- git-odb/src/pack/index/write/mod.rs | 2 +- git-odb/src/pack/tree/iter.rs | 208 +++++---------------- git-odb/src/pack/tree/mod.rs | 107 ++++++----- git-odb/src/pack/tree/traverse/mod.rs | 14 +- git-odb/src/pack/tree/traverse/resolve.rs | 6 +- 7 files changed, 123 insertions(+), 224 deletions(-) diff --git a/git-odb/src/pack/index/traverse/indexed.rs b/git-odb/src/pack/index/traverse/indexed.rs index 63be1ad2396..4dce107f423 100644 --- a/git-odb/src/pack/index/traverse/indexed.rs +++ b/git-odb/src/pack/index/traverse/indexed.rs @@ -1,3 +1,5 @@ +use std::collections::VecDeque; + use super::{Error, SafetyCheck}; use crate::pack::{ self, @@ -157,7 +159,7 @@ impl From for EntryWithDefault { } } -fn digest_statistics(items: Vec>) -> index::traverse::Outcome { +fn digest_statistics(items: VecDeque>) -> index::traverse::Outcome { let mut res = index::traverse::Outcome::default(); let average = &mut res.average; for item in &items { diff --git a/git-odb/src/pack/index/write/encode.rs b/git-odb/src/pack/index/write/encode.rs index c8f2b517d40..335907cf304 100644 --- a/git-odb/src/pack/index/write/encode.rs +++ b/git-odb/src/pack/index/write/encode.rs @@ -4,11 +4,11 @@ use crate::{ }; use byteorder::{BigEndian, WriteBytesExt}; use git_features::progress::{self, Progress}; -use std::{cmp::Ordering, io}; +use std::{cmp::Ordering, collections::VecDeque, io}; pub(crate) fn write_to( out: impl io::Write, - entries_sorted_by_oid: Vec>, + entries_sorted_by_oid: VecDeque>, pack_hash: &git_hash::ObjectId, kind: pack::index::Version, mut progress: impl Progress, @@ -36,7 +36,7 @@ pub(crate) fn write_to( const HIGH_BIT: u32 = 0x8000_0000; let needs_64bit_offsets = - entries_sorted_by_oid.last().expect("at least one pack entry").offset > LARGE_OFFSET_THRESHOLD; + entries_sorted_by_oid.back().expect("at least one pack entry").offset > LARGE_OFFSET_THRESHOLD; let mut fan_out_be = [0u32; 256]; progress.init(Some(4), progress::steps()); let start = std::time::Instant::now(); diff --git a/git-odb/src/pack/index/write/mod.rs b/git-odb/src/pack/index/write/mod.rs index f4bc799520b..0a535f7300b 100644 --- a/git-odb/src/pack/index/write/mod.rs +++ b/git-odb/src/pack/index/write/mod.rs @@ -185,7 +185,7 @@ impl pack::index::File { { let _progress = root_progress.add_child("sorting by id"); - items.sort_by_key(|e| e.data.id); + items.make_contiguous().sort_by_key(|e| e.data.id); } root_progress.inc(); diff --git a/git-odb/src/pack/tree/iter.rs b/git-odb/src/pack/tree/iter.rs index 6d9a4a2e6af..6d0bc22afcd 100644 --- a/git-odb/src/pack/tree/iter.rs +++ b/git-odb/src/pack/tree/iter.rs @@ -3,191 +3,83 @@ use crate::{ pack::tree::{Item, Tree}, }; -/// All the **unsafe** bits to support parallel iteration with write access +/// Iteration impl Tree { - #[allow(unsafe_code)] - /// # SAFETY - /// Called from node with is guaranteed to not be aliasing with any other node. - /// Note that we are iterating nodes that we are affecting here, but that only affects the - /// 'is_root' field fo the item, not the data field that we are writing here. - /// For all details see `from_node_take_entry()`. - unsafe fn put_data(&self, index: usize, data: T) { - let items_mut: &mut Vec> = &mut *(self.items.get()); - items_mut.get_unchecked_mut(index).data = data; - } + /// Return an iterator over chunks of roots. Roots are not children themselves, they have no parents. + pub fn iter_root_chunks(&mut self, chunk_size: usize) -> impl Iterator> + '_ { + let (front, back) = self.items.as_mut_slices(); + if front.len() != self.roots { + // This should not happen unless we added more nodes than were declared in the + // constructor. + panic!("item deque has been resized"); + } - #[allow(unsafe_code)] - /// # SAFETY - /// Similar to `from_node_put_data(…)` - unsafe fn offset(&self, index: usize) -> u64 { - let items: &Vec> = &*(self.items.get()); - items.get_unchecked(index).offset + front.chunks_mut(chunk_size).map(move |c| Chunk { + inner: c.iter_mut(), + children: back as *mut [Item], + }) } +} - #[allow(unsafe_code)] - /// # SAFETY - /// Similar to `from_node_put_data(…)` - unsafe fn entry_slice(&self, index: usize) -> std::ops::Range { - let items: &Vec> = &*(self.items.get()); - let start = items.get_unchecked(index).offset; - let end = items - .get(index + 1) - .map(|e| e.offset) - .or(self.pack_entries_end) - .expect("traversal(…) to have set this value (BUG)"); - start..end - } +/// A chunk returned by `iter_root_chunks`, which can be iterated over to get [`Node`]s. +pub struct Chunk<'a, T> { + inner: std::slice::IterMut<'a, Item>, + children: *mut [Item], +} - #[allow(unsafe_code)] - /// # SAFETY - /// A tree is a data structure without cycles, and we assure of that by verifying all input. - /// A node as identified by index can only be traversed once using the Chunks iterator. - /// When the iterator is created, this instance cannot be mutated anymore nor can it be read. - /// That iterator is only handed out once. - /// `Node` instances produced by it consume themselves when iterating their children, allowing them to be - /// used only once, recursively. Again, traversal is always forward and consuming, making it impossible to - /// alias multiple nodes in the tree. - /// It's safe for multiple threads to hold different chunks, as they are guaranteed to be non-overlapping and unique. - /// If the tree is accessed after iteration, it will panic as no mutation is allowed anymore, nor is - unsafe fn take_entry(&self, index: usize) -> (T, Vec) - where - T: Default, - { - let items_mut: &mut Vec> = &mut *(self.items.get()); - let item = items_mut.get_unchecked_mut(index); - let children = std::mem::take(&mut item.children); - let data = std::mem::take(&mut item.data); - (data, children) - } +// SAFETY: The raw pointer is uniquely materialized in `Node::into_child_iter`. +#[allow(unsafe_code)] +unsafe impl<'a, T> Send for Chunk<'a, T> {} - #[allow(unsafe_code)] - /// # SAFETY - /// As `take_entry(…)` - but this one only takes if the data of Node is a root - unsafe fn from_iter_take_entry_if_root(&self, index: usize) -> Option<(T, Vec)> - where - T: Default, - { - let items_mut: &mut Vec> = &mut *(self.items.get()); - let item = items_mut.get_unchecked_mut(index); - if item.is_root { - let children = std::mem::take(&mut item.children); - let data = std::mem::take(&mut item.data); - Some((data, children)) - } else { - None - } - } -} +impl<'a, T> Iterator for Chunk<'a, T> { + type Item = Node<'a, T>; -/// Iteration -impl Tree { - /// Return an iterator over chunks of roots. Roots are not children themselves, they have no parents. - pub fn iter_root_chunks(&mut self, chunk_size: usize) -> Chunks<'_, T> { - Chunks { - tree: self, - chunk_size, - cursor: 0, - } + fn next(&mut self) -> Option { + self.inner.next().map(|item| { + Node { + item, + children: self.children, + } + }) } } -/// An item returned by a [`Chunks`] iterator, allowing access to the `data` stored alongside nodes in a [`Tree`] +/// An item returned by `iter_root_chunks`, allowing access to the `data` stored alongside nodes in a [`Tree`]. pub struct Node<'a, T> { - tree: &'a Tree, - index: usize, - children: Vec, - /// The custom data attached to each node of the tree whose ownership was transferred into the iteration. - pub data: T, + item: &'a mut Item, + children: *mut [Item], } -impl<'a, T> Node<'a, T> -where - T: Default, -{ +impl<'a, T> Node<'a, T> { /// Returns the offset into the pack at which the `Node`s data is located. pub fn offset(&self) -> u64 { - #[allow(unsafe_code)] - // SAFETY: The index is valid as it was controlled by `add_child(…)`, then see `take_entry(…)` - unsafe { - self.tree.offset(self.index) - } + self.item.offset } /// Returns the slice into the data pack at which the pack entry is located. pub fn entry_slice(&self) -> pack::data::EntryRange { - #[allow(unsafe_code)] - // SAFETY: The index is valid as it was controlled by `add_child(…)`, then see `take_entry(…)` - unsafe { - self.tree.entry_slice(self.index) - } + self.item.offset..self.item.next_offset + } + + /// Returns the node data associated with this node. + pub fn data(&mut self) -> &mut T { + &mut self.item.data } - /// Write potentially changed `Node` data back into the [`Tree`] and transform this `Node` into an iterator - /// over its `Node`s children. + /// Transform this `Node` into an iterator over its children. /// /// Children are `Node`s referring to pack entries whose base object is this pack entry. - pub fn store_changes_then_into_child_iter(self) -> impl Iterator> { - #[allow(unsafe_code)] - // SAFETY: The index is valid as it was controlled by `add_child(…)`, then see `take_entry(…)` - unsafe { - self.tree.put_data(self.index, self.data) - }; - let Self { tree, children, .. } = self; - children.into_iter().map(move |index| { - // SAFETY: The index is valid as it was controlled by `add_child(…)`, then see `take_entry(…)` + pub fn into_child_iter(self) -> impl Iterator> + 'a { + let children = self.children; + self.item.children.iter().map(move |&index| { + // SAFETY: The children array is alive by the 'a lifetime. + // SAFETY: The index is a valid index into the children array. + // SAFETY: The resulting mutable pointer cannot be yielded by any other node. #[allow(unsafe_code)] - let (data, children) = unsafe { tree.take_entry(index) }; Node { - tree, - index, + item: unsafe { &mut *(children as *mut Item).add(index) }, children, - data, } }) } } - -/// An iterator over chunks of [`Node`]s in a [`Tree`] -pub struct Chunks<'a, T> { - tree: &'a Tree, - chunk_size: usize, - cursor: usize, -} - -impl<'a, T> Iterator for Chunks<'a, T> -where - T: Default, -{ - type Item = Vec>; - - fn next(&mut self) -> Option { - if self.cursor == self.tree.one_past_last_seen_root { - return None; - } - let mut items_remaining = self.chunk_size; - let mut res = Vec::with_capacity(self.chunk_size); - - while items_remaining > 0 && self.cursor < self.tree.one_past_last_seen_root { - // SAFETY: The index is valid as the cursor cannot surpass the amount of items. `one_past_last_seen_root` - // is guaranteed to be self.tree.items.len() at most, or smaller. - // Then see `take_entry_if_root(…)` - #[allow(unsafe_code)] - if let Some((data, children)) = unsafe { self.tree.from_iter_take_entry_if_root(self.cursor) } { - res.push(Node { - tree: self.tree, - index: self.cursor, - children, - data, - }); - items_remaining -= 1; - } - self.cursor += 1; - } - - if res.is_empty() { - None - } else { - Some(res) - } - } -} diff --git a/git-odb/src/pack/tree/mod.rs b/git-odb/src/pack/tree/mod.rs index f9b21ec0e04..b781303b6c5 100644 --- a/git-odb/src/pack/tree/mod.rs +++ b/git-odb/src/pack/tree/mod.rs @@ -1,4 +1,4 @@ -use std::cell::UnsafeCell; +use std::collections::VecDeque; /// Returned when using various methods on a [`Tree`] #[derive(thiserror::Error, Debug)] @@ -23,7 +23,7 @@ pub enum Error { } mod iter; -pub use iter::{Chunks, Node}; +pub use iter::{Chunk, Node}; /// pub mod traverse; @@ -34,27 +34,22 @@ pub mod from_offsets; pub struct Item { /// The offset into the pack file at which the pack entry's data is located. pub offset: u64, - /// If true, this object may have children but is not a child itself. It's thus the root of a tree. - is_root: bool, + /// The offset of the next item in the pack file. + pub next_offset: u64, /// Data to store with each Item, effectively data associated with each entry in a pack. pub data: T, children: Vec, } /// A tree that allows one-time iteration over all nodes and their children, consuming it in the process, /// while being shareable among threads without a lock. -/// It does this by making the run-time guarantee that iteration only happens once. +/// It does this by making the guarantee that iteration only happens once. pub struct Tree { - items: UnsafeCell>>, - last_added_offset: u64, - one_past_last_seen_root: usize, - pack_entries_end: Option, + /// Roots are first, then children. + items: VecDeque>, + roots: usize, + last_index: usize, } -/// SAFETY: We solemnly swear…that this is sync because without the unsafe cell, it is also sync. -/// But that's really the only reason why I would dare to know. -#[allow(unsafe_code)] -unsafe impl Sync for Tree {} - impl Tree { /// Instantiate a empty tree capable of storing `num_objects` amounts of items. pub fn with_capacity(num_objects: usize) -> Result { @@ -62,69 +57,79 @@ impl Tree { return Err(Error::InvariantNonEmpty); } Ok(Tree { - items: UnsafeCell::new(Vec::with_capacity(num_objects)), - last_added_offset: 0, - one_past_last_seen_root: 0, - pack_entries_end: None, + items: VecDeque::with_capacity(num_objects), + roots: 0, + last_index: 0, }) } - fn assert_is_incrementing(&mut self, offset: u64) -> Result { - if offset > self.last_added_offset { - self.last_added_offset = offset; - Ok(offset) - } else { - Err(Error::InvariantIncreasingPackOffset { - last_pack_offset: self.last_added_offset, + fn assert_is_incrementing(&mut self, offset: u64) -> Result<(), Error> { + if self.items.is_empty() { + return Ok(()); + } + let last_offset = self.items[self.last_index].offset; + if offset <= last_offset { + return Err(Error::InvariantIncreasingPackOffset { + last_pack_offset: last_offset, pack_offset: offset, - }) + }); + } + self.items[self.last_index].next_offset = offset; + Ok(()) + } + + fn set_pack_entries_end(&mut self, pack_entries_end: u64) { + if !self.items.is_empty() { + self.items[self.last_index].next_offset = pack_entries_end; } } /// Add a new root node, one that only has children but is not a child itself, at the given pack `offset` and associate /// custom `data` with it. pub fn add_root(&mut self, offset: u64, data: T) -> Result<(), Error> { - // SAFETY: Because we passed the assertion above which implies no other access is possible as per - // standard borrow check rules. - #[allow(unsafe_code)] - let items = unsafe { &mut *(self.items.get()) }; - let offset = self.assert_is_incrementing(offset)?; - items.push(Item { + self.assert_is_incrementing(offset)?; + self.last_index = 0; + self.items.push_front(Item { offset, + next_offset: 0, data, - is_root: true, - children: Default::default(), + children: Vec::new(), }); - self.one_past_last_seen_root = items.len(); + self.roots += 1; Ok(()) } /// Add a child of the item at `base_offset` which itself resides at pack `offset` and associate custom `data` with it. pub fn add_child(&mut self, base_offset: u64, offset: u64, data: T) -> Result<(), Error> { - // SAFETY: Because we passed the assertion above which implies no other access is possible as per - // standard borrow check rules. - #[allow(unsafe_code)] - let items = unsafe { &mut *(self.items.get()) }; - let offset = self.assert_is_incrementing(offset)?; - let base_index = items.binary_search_by_key(&base_offset, |e| e.offset).map_err(|_| { - Error::InvariantBasesBeforeDeltasNeedThem { + self.assert_is_incrementing(offset)?; + let (roots, children) = self.items.as_mut_slices(); + if roots.len() != self.roots { + // This should not happen unless we added more nodes than were declared in the + // constructor. + panic!("item deque has been resized"); + } + if let Ok(i) = children.binary_search_by_key(&base_offset, |i| i.offset) { + children[i].children.push(children.len()); + } else if let Ok(i) = roots.binary_search_by(|i| base_offset.cmp(&i.offset)) { + roots[i].children.push(children.len()); + } else { + return Err(Error::InvariantBasesBeforeDeltasNeedThem { delta_pack_offset: offset, base_pack_offset: base_offset, - } - })?; - let child_index = items.len(); - items[base_index].children.push(child_index); - items.push(Item { - is_root: false, + }); + } + self.last_index = self.items.len(); + self.items.push_back(Item { offset, + next_offset: 0, data, - children: Default::default(), + children: Vec::new(), }); Ok(()) } /// Transform this `Tree` into its items. - pub fn into_items(self) -> Vec> { - self.items.into_inner() + pub fn into_items(self) -> VecDeque> { + self.items } } diff --git a/git-odb/src/pack/tree/traverse/mod.rs b/git-odb/src/pack/tree/traverse/mod.rs index 72805183c53..beffb53f47a 100644 --- a/git-odb/src/pack/tree/traverse/mod.rs +++ b/git-odb/src/pack/tree/traverse/mod.rs @@ -1,3 +1,5 @@ +use std::collections::VecDeque; + use crate::{ pack, pack::data::EntryRange, @@ -79,20 +81,18 @@ where pack_entries_end: u64, new_thread_state: impl Fn() -> S + Send + Sync, inspect_object: MBFN, - ) -> Result>, Error> + ) -> Result>, Error> where F: for<'r> Fn(EntryRange, &'r mut Vec) -> Option<()> + Send + Sync, P: Progress + Send, MBFN: Fn(&mut T, &mut

::SubProgress, Context<'_, S>) -> Result<(), E> + Send + Sync, E: std::error::Error + Send + Sync + 'static, { - self.pack_entries_end = Some(pack_entries_end); + self.set_pack_entries_end(pack_entries_end); let (chunk_size, thread_limit, _) = parallel::optimize_chunk_size_and_thread_limit(1, None, thread_limit, None); let object_progress = parking_lot::Mutex::new(object_progress); - // SAFETY: We are owning 'self', and it's the UnsafeCell which we are supposed to use requiring unsafe on every access now. - #[allow(unsafe_code)] - let num_objects = unsafe { (*self.items.get()).len() } as u32; + let num_objects = self.items.len(); in_parallel_if( should_run_in_parallel, self.iter_root_chunks(chunk_size), @@ -122,10 +122,10 @@ impl<'a, P> Reducer<'a, P> where P: Progress, { - pub fn new(num_objects: u32, progress: &'a parking_lot::Mutex

, mut size_progress: P) -> Self { + pub fn new(num_objects: usize, progress: &'a parking_lot::Mutex

, mut size_progress: P) -> Self { progress .lock() - .init(Some(num_objects as usize), progress::count("objects")); + .init(Some(num_objects), progress::count("objects")); size_progress.init(None, progress::bytes()); Reducer { item_count: 0, diff --git a/git-odb/src/pack/tree/traverse/resolve.rs b/git-odb/src/pack/tree/traverse/resolve.rs index ec428739f6b..be1d74e0487 100644 --- a/git-odb/src/pack/tree/traverse/resolve.rs +++ b/git-odb/src/pack/tree/traverse/resolve.rs @@ -6,7 +6,7 @@ use git_features::progress::{unit, Progress}; use std::{cell::RefCell, collections::BTreeMap}; pub(crate) fn deltas( - nodes: Vec>, + nodes: pack::tree::Chunk<'_, T>, (bytes_buf, ref mut progress, state): &mut (Vec, P, S), resolve: F, modify_base: MBFN, @@ -57,7 +57,7 @@ where }; modify_base( - &mut base.data, + base.data(), progress, Context { entry: &base_entry, @@ -71,7 +71,7 @@ where num_objects += 1; decompressed_bytes += base_bytes.len() as u64; progress.inc(); - for child in base.store_changes_then_into_child_iter() { + for child in base.into_child_iter() { let (mut child_entry, entry_end, delta_bytes) = decompress_from_resolver(child.entry_slice())?; let (base_size, consumed) = pack::data::delta::decode_header_size(&delta_bytes); let mut header_ofs = consumed; From a341995e6014b6ed0e43ae94fa1152aed6fcfd89 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 8 May 2021 17:48:10 +0800 Subject: [PATCH 2/3] Fix formatting --- git-odb/src/pack/tree/iter.rs | 10 ++++------ git-odb/src/pack/tree/traverse/mod.rs | 4 +--- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/git-odb/src/pack/tree/iter.rs b/git-odb/src/pack/tree/iter.rs index 6d0bc22afcd..8941df963bf 100644 --- a/git-odb/src/pack/tree/iter.rs +++ b/git-odb/src/pack/tree/iter.rs @@ -6,7 +6,7 @@ use crate::{ /// Iteration impl Tree { /// Return an iterator over chunks of roots. Roots are not children themselves, they have no parents. - pub fn iter_root_chunks(&mut self, chunk_size: usize) -> impl Iterator> + '_ { + pub fn iter_root_chunks(&mut self, chunk_size: usize) -> impl Iterator> + '_ { let (front, back) = self.items.as_mut_slices(); if front.len() != self.roots { // This should not happen unless we added more nodes than were declared in the @@ -35,11 +35,9 @@ impl<'a, T> Iterator for Chunk<'a, T> { type Item = Node<'a, T>; fn next(&mut self) -> Option { - self.inner.next().map(|item| { - Node { - item, - children: self.children, - } + self.inner.next().map(|item| Node { + item, + children: self.children, }) } } diff --git a/git-odb/src/pack/tree/traverse/mod.rs b/git-odb/src/pack/tree/traverse/mod.rs index beffb53f47a..73aa0005c4c 100644 --- a/git-odb/src/pack/tree/traverse/mod.rs +++ b/git-odb/src/pack/tree/traverse/mod.rs @@ -123,9 +123,7 @@ where P: Progress, { pub fn new(num_objects: usize, progress: &'a parking_lot::Mutex

, mut size_progress: P) -> Self { - progress - .lock() - .init(Some(num_objects), progress::count("objects")); + progress.lock().init(Some(num_objects), progress::count("objects")); size_progress.init(None, progress::bytes()); Reducer { item_count: 0, From a9e4feb0a81894568be730603446e2d061dd558f Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 8 May 2021 18:02:08 +0800 Subject: [PATCH 3/3] refactor --- git-odb/src/pack/tree/iter.rs | 10 +++++----- git-odb/src/pack/tree/mod.rs | 10 +++++----- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/git-odb/src/pack/tree/iter.rs b/git-odb/src/pack/tree/iter.rs index 8941df963bf..9a690304043 100644 --- a/git-odb/src/pack/tree/iter.rs +++ b/git-odb/src/pack/tree/iter.rs @@ -8,11 +8,11 @@ impl Tree { /// Return an iterator over chunks of roots. Roots are not children themselves, they have no parents. pub fn iter_root_chunks(&mut self, chunk_size: usize) -> impl Iterator> + '_ { let (front, back) = self.items.as_mut_slices(); - if front.len() != self.roots { - // This should not happen unless we added more nodes than were declared in the - // constructor. - panic!("item deque has been resized"); - } + assert_eq!( + front.len(), + self.roots, + "This should not happen unless we added more nodes than were declared in the constructor" + ); front.chunks_mut(chunk_size).map(move |c| Chunk { inner: c.iter_mut(), diff --git a/git-odb/src/pack/tree/mod.rs b/git-odb/src/pack/tree/mod.rs index b781303b6c5..ee347122379 100644 --- a/git-odb/src/pack/tree/mod.rs +++ b/git-odb/src/pack/tree/mod.rs @@ -103,11 +103,11 @@ impl Tree { pub fn add_child(&mut self, base_offset: u64, offset: u64, data: T) -> Result<(), Error> { self.assert_is_incrementing(offset)?; let (roots, children) = self.items.as_mut_slices(); - if roots.len() != self.roots { - // This should not happen unless we added more nodes than were declared in the - // constructor. - panic!("item deque has been resized"); - } + assert_eq!( + roots.len(), + self.roots, + "item deque has been resized, maybe we added more nodes than we declared in the constructor?" + ); if let Ok(i) = children.binary_search_by_key(&base_offset, |i| i.offset) { children[i].children.push(children.len()); } else if let Ok(i) = roots.binary_search_by(|i| base_offset.cmp(&i.offset)) {