Skip to content

Commit cededfc

Browse files
eeeebbbbrrrrstuhood
authored andcommitted
perf: lazily open positions file (#54)
Not all queries use positions and it's okay if we (from the perspective of `pg_search`, anyways) defer opening/loading them until they're first needed. This is probably completely wrong for a mmap-based Directory, but we (again, `pg_search`) decided long ago that we don't care about that use case. This saves a lot of disk I/O when an index has lots of segments and the query doesn't need positions. As a drive by, make sure a random Vec has enough space before pushing items to it. This showed up in the profiler, believe it or not.
1 parent 4317d5c commit cededfc

4 files changed

Lines changed: 62 additions & 15 deletions

File tree

common/src/file_slice.rs

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use std::fs::File;
22
use std::ops::{Deref, Range, RangeBounds};
33
use std::path::Path;
4-
use std::sync::Arc;
4+
use std::sync::{Arc, OnceLock};
55
use std::{fmt, io};
66

77
use async_trait::async_trait;
@@ -339,6 +339,27 @@ impl FileHandle for OwnedBytes {
339339
}
340340
}
341341

342+
pub struct DeferredFileSlice {
343+
opener: Arc<dyn Fn() -> io::Result<FileSlice> + Send + Sync + 'static>,
344+
file_slice: OnceLock<std::io::Result<FileSlice>>,
345+
}
346+
347+
impl DeferredFileSlice {
348+
pub fn new(opener: impl Fn() -> io::Result<FileSlice> + Send + Sync + 'static) -> Self {
349+
DeferredFileSlice {
350+
opener: Arc::new(opener),
351+
file_slice: OnceLock::default(),
352+
}
353+
}
354+
355+
pub fn open(&self) -> io::Result<&FileSlice> {
356+
match self.file_slice.get_or_init(|| (self.opener)()) {
357+
Ok(file_slice) => Ok(file_slice),
358+
Err(e) => Err(io::Error::new(io::ErrorKind::Other, e.to_string())),
359+
}
360+
}
361+
}
362+
342363
#[cfg(test)]
343364
mod tests {
344365
use std::io;

src/index/inverted_index_reader.rs

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
use std::io;
22

3+
use common::file_slice::DeferredFileSlice;
34
use common::json_path_writer::JSON_END_OF_PATH;
45
use common::{BinarySerializable, ByteCount};
56
#[cfg(feature = "quickwit")]
@@ -30,7 +31,7 @@ use crate::termdict::TermDictionary;
3031
pub struct InvertedIndexReader {
3132
termdict: TermDictionary,
3233
postings_file_slice: FileSlice,
33-
positions_file_slice: FileSlice,
34+
positions_file_slice: DeferredFileSlice,
3435
record_option: IndexRecordOption,
3536
total_num_tokens: u64,
3637
}
@@ -66,7 +67,7 @@ impl InvertedIndexReader {
6667
pub(crate) fn new(
6768
termdict: TermDictionary,
6869
postings_file_slice: FileSlice,
69-
positions_file_slice: FileSlice,
70+
positions_file_slice: DeferredFileSlice,
7071
record_option: IndexRecordOption,
7172
) -> io::Result<InvertedIndexReader> {
7273
let (total_num_tokens_slice, postings_body) = postings_file_slice.split(8);
@@ -86,7 +87,7 @@ impl InvertedIndexReader {
8687
InvertedIndexReader {
8788
termdict: TermDictionary::empty(),
8889
postings_file_slice: FileSlice::empty(),
89-
positions_file_slice: FileSlice::empty(),
90+
positions_file_slice: DeferredFileSlice::new(|| Ok(FileSlice::empty())),
9091
record_option,
9192
total_num_tokens: 0u64,
9293
}
@@ -233,6 +234,7 @@ impl InvertedIndexReader {
233234
if option.has_positions() {
234235
let positions_data = self
235236
.positions_file_slice
237+
.open()?
236238
.read_bytes_slice(term_info.positions_range.clone())?;
237239
let position_reader = PositionReader::open(positions_data)?;
238240
Some(position_reader)
@@ -342,6 +344,7 @@ impl InvertedIndexReader {
342344
if with_positions {
343345
let positions = self
344346
.positions_file_slice
347+
.open()?
345348
.read_bytes_slice_async(term_info.positions_range.clone());
346349
futures_util::future::try_join(postings, positions).await?;
347350
} else {
@@ -384,6 +387,7 @@ impl InvertedIndexReader {
384387
if with_positions {
385388
let positions = self
386389
.positions_file_slice
390+
.open()?
387391
.read_bytes_slice_async(positions_range);
388392
futures_util::future::try_join(postings, positions).await?;
389393
} else {
@@ -478,7 +482,7 @@ impl InvertedIndexReader {
478482
pub async fn warm_postings_full(&self, with_positions: bool) -> io::Result<()> {
479483
self.postings_file_slice.read_bytes_async().await?;
480484
if with_positions {
481-
self.positions_file_slice.read_bytes_async().await?;
485+
self.positions_file_slice.open()?.read_bytes_async().await?;
482486
}
483487
Ok(())
484488
}

src/index/segment_reader.rs

Lines changed: 29 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ use std::path::PathBuf;
44
use std::sync::{Arc, OnceLock, RwLock};
55
use std::{fmt, io};
66

7+
use common::file_slice::DeferredFileSlice;
78
use common::{ByteCount, HasLen};
89
use fnv::FnvHashMap;
910
use itertools::Itertools;
@@ -237,19 +238,38 @@ impl SegmentReader {
237238
))
238239
})?;
239240

240-
let positions_file = self.positions_composite().open_read(field).ok_or_else(|| {
241-
let error_msg = format!(
242-
"Failed to open field {:?}'s positions in the composite file. Has the schema been \
243-
modified?",
244-
field_entry.name()
245-
);
246-
DataCorruption::comment_only(error_msg)
247-
})?;
241+
// not all queries require positions.
242+
// we can defer opening the file until needed
243+
let positions_file_opener = {
244+
let path = self.relative_path(SegmentComponent::Positions);
245+
let directory = self.index.directory().clone();
246+
let field_entry = field_entry.clone();
247+
move || {
248+
let composite_file = if let Ok(positions_file) = &directory.open_read(&path) {
249+
CompositeFile::open(&positions_file)
250+
.expect("should be able to open positions composite component")
251+
} else {
252+
CompositeFile::empty()
253+
};
254+
255+
composite_file.open_read(field).ok_or_else(|| {
256+
let error_msg = format!(
257+
"Failed to open field {:?}'s positions in the composite file. Has the \
258+
schema been modified?",
259+
field_entry.name()
260+
);
261+
io::Error::new(
262+
io::ErrorKind::InvalidData,
263+
format!("{:?}", DataCorruption::comment_only(error_msg)),
264+
)
265+
})
266+
}
267+
};
248268

249269
let inv_idx_reader = Arc::new(InvertedIndexReader::new(
250270
TermDictionary::open(termdict_file)?,
251271
postings_file,
252-
positions_file,
272+
DeferredFileSlice::new(positions_file_opener),
253273
record_option,
254274
)?);
255275

src/termdict/sstable_termdict/mod.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,10 +158,12 @@ impl ValueReader for TermInfoValueReader {
158158

159159
fn load(&mut self, mut data: &[u8]) -> io::Result<usize> {
160160
let len_before = data.len();
161-
self.term_infos.clear();
162161
let num_els = VInt::deserialize_u64(&mut data)?;
163162
let mut postings_start = VInt::deserialize_u64(&mut data)? as usize;
164163
let mut positions_start = VInt::deserialize_u64(&mut data)? as usize;
164+
165+
self.term_infos.clear();
166+
self.term_infos.reserve_exact(num_els as usize);
165167
for _ in 0..num_els {
166168
let doc_freq = VInt::deserialize_u64(&mut data)? as u32;
167169
let postings_num_bytes = VInt::deserialize_u64(&mut data)?;

0 commit comments

Comments
 (0)