Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add sparse codec #1723

Merged
merged 7 commits into from
Dec 20, 2022
Merged

add sparse codec #1723

merged 7 commits into from
Dec 20, 2022

Conversation

PSeitz
Copy link
Contributor

@PSeitz PSeitz commented Dec 13, 2022

test null_index::dense::bench::bench_dense_codec_translate_dense_to_orig_90percent_filled_full_scan                ... bench:  50,590,455 ns/iter (+/- 24,920,729)
test null_index::dense::bench::bench_dense_codec_translate_orig_to_dense_10percent_filled_random_stride            ... bench:     542,766 ns/iter (+/- 8,079)
test null_index::dense::bench::bench_dense_codec_translate_orig_to_dense_full_scan_10percent                       ... bench:  10,259,153 ns/iter (+/- 6,611,325)
test null_index::dense::bench::bench_dense_codec_translate_orig_to_dense_full_scan_90percent                       ... bench:  10,515,926 ns/iter (+/- 7,147,110)
test null_index::sparse::bench::bench_sparse_codec_translate_orig_to_sparse_10percent_filled_random_stride         ... bench:   5,232,288 ns/iter (+/- 3,810,411)
test null_index::sparse::bench::bench_sparse_codec_translate_orig_to_sparse_1percent_filled_random_stride          ... bench:   1,166,540 ns/iter (+/- 2,005,600)
test null_index::sparse::bench::bench_sparse_codec_translate_orig_to_sparse_5percent_filled_random_stride          ... bench:   1,318,146 ns/iter (+/- 1,663,206)
test null_index::sparse::bench::bench_sparse_codec_translate_orig_to_sparse_full_scan_10percent                    ... bench:  32,506,578 ns/iter (+/- 66,071,241)
test null_index::sparse::bench::bench_sparse_codec_translate_orig_to_sparse_full_scan_1percent                     ... bench:  15,320,323 ns/iter (+/- 479,625)
test null_index::sparse::bench::bench_sparse_codec_translate_sparse_to_orig_1percent_filled_full_scan              ... bench:      13,582 ns/iter (+/- 446)
test null_index::sparse::bench::bench_sparse_codec_translate_sparse_to_orig_1percent_filled_random_stride          ... bench:       1,031 ns/iter (+/- 7)
test null_index::sparse::bench::bench_sparse_codec_translate_sparse_to_orig_1percent_filled_random_stride_big_step ... bench:         109 ns/iter (+/- 0)

@PSeitz PSeitz force-pushed the sparse_dense_index branch 2 times, most recently from fbf4e86 to f7e5efa Compare December 14, 2022 08:03
@PSeitz PSeitz force-pushed the sparse_dense_index branch 3 times, most recently from 615c0c2 to 7bac90f Compare December 14, 2022 09:09
@codecov-commenter
Copy link

codecov-commenter commented Dec 14, 2022

Codecov Report

Merging #1723 (0d1e901) into main (f9171a3) will increase coverage by 0.00%.
The diff coverage is 96.66%.

@@           Coverage Diff            @@
##             main    #1723    +/-   ##
========================================
  Coverage   94.06%   94.07%            
========================================
  Files         262      263     +1     
  Lines       49994    50518   +524     
========================================
+ Hits        47028    47525   +497     
- Misses       2966     2993    +27     
Impacted Files Coverage Δ
fastfield_codecs/src/null_index/mod.rs 100.00% <ø> (ø)
fastfield_codecs/src/null_index/dense.rs 99.06% <93.33%> (-0.32%) ⬇️
fastfield_codecs/src/null_index/sparse.rs 96.74% <96.74%> (ø)
fastfield_codecs/src/serialize.rs 86.56% <100.00%> (ø)
src/indexer/segment_updater.rs 94.40% <0.00%> (-1.05%) ⬇️
src/store/index/mod.rs 97.83% <0.00%> (-0.55%) ⬇️
src/schema/schema.rs 98.78% <0.00%> (-0.14%) ⬇️
sstable/src/block_reader.rs 82.14% <0.00%> (+1.78%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

// The number of vals so far
let mut offset = 0;
let mut sparse_codec_blocks = Vec::new();
let num_blocks = u16::from_le_bytes([data[data.len() - 2], data[data.len() - 1]]);
Copy link
Collaborator

@fulmicoton fulmicoton Dec 15, 2022

Choose a reason for hiding this comment

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

It would be nice to introduce

#[inline(always)]
pub fn read_u16_at_offset(data: &[u8], offset: usize) -> u16 {
    assert!(offset+1 < data.len());
    unsafe { read_u16_at_offset_unsafe(data, offset) }
}

#[inline(always)]
pub unsafe fn read_u16_at_offset_unsafe(data: &[u8], offset: usize) -> u16 {
    unsafe {
        let u16_ptr: *const [u8; 2] = data.as_ptr().offset(offset as isize) as *const [u8; 2];
        let u16_bytes: [u8; 2] = u16_ptr.read_unaligned();
        u16::from_le_bytes(u16_bytes)
    }
}

And use it everywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would only use unsafe when we can see the difference in the benchmark

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's forget the unsafe part then, we already have a different implementation of this in one place...
Can we extract it as a function mark it as inline and use it everywhere in this file.


/// Splits a idx into block index and value in the block
fn split_in_block_idx_and_val_in_block(idx: u32) -> (u16, u16) {
let val_in_block = (idx % u16::MAX as u32) as u16;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let val_in_block = (idx % u16::MAX as u32) as u16;
let val_in_block = (idx % u16::MAX as u32) as u16;

That's a bug isn't it? u16::MAX is incorrect. We want 1 << 16

Copy link
Collaborator

Choose a reason for hiding this comment

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

discussed offline, that was a measured decision to deal with the fact that
a len in a block defined that way takes values in [0..u16::MAX], so that it cannot be represented as a u16.

let (block_idx, val_in_block) = split_in_block_idx_and_val_in_block(idx);
// There may be trailing nulls without data, those are not stored as blocks. It would be
// possible to create empty blocks, but for that we would need to serialize the number of
// values or pass them when opening
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rather do that actually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would add some fixed bytes to every sparse codec field


/// Return the number of non-null values in an index
pub fn num_non_null_vals(&self) -> u32 {
self.blocks.last().map(|block| block.offset).unwrap_or(0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We seem to use num_null_vals or num_non_null_vals in different places. (Index traits)

I'd rather settle to num_non_null_vals everywhere... What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, let's stick to that

Copy link
Collaborator

Choose a reason for hiding this comment

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

can you make the modification in the ValueIndex trait(s?)

Copy link
Collaborator

@fulmicoton fulmicoton left a comment

Choose a reason for hiding this comment

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

See comments

fn value_addr(idx: u32) -> ValueAddr {
/// Static assert number elements per block this method expects
#[allow(clippy::assertions_on_constants)]
const _: () = assert!(ELEMENTS_PER_BLOCK == (1 << 16));
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's the first time I see this trick. Interesting.

Copy link
Collaborator

@fulmicoton fulmicoton left a comment

Choose a reason for hiding this comment

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

Good work. Please have a look at clippy comments before merging.

PSeitz and others added 3 commits December 20, 2022 17:16
Co-authored-by: Paul Masurel <paul@quickwit.io>
Co-authored-by: Paul Masurel <paul@quickwit.io>
@PSeitz PSeitz merged commit 2ac1cc2 into main Dec 20, 2022
@PSeitz PSeitz deleted the sparse_dense_index branch December 20, 2022 14:30
This was referenced Jan 13, 2023
Hodkinson pushed a commit to Hodkinson/tantivy that referenced this pull request Jan 30, 2023
* add sparse codec

* Apply suggestions from code review

Co-authored-by: Paul Masurel <paul@quickwit.io>

* Apply suggestions from code review

Co-authored-by: Paul Masurel <paul@quickwit.io>

* Apply suggestions from code review

Co-authored-by: Paul Masurel <paul@quickwit.io>

* add the -1 u16 fix for metadata num_vals

* add dense block encoding to sparse codec

* add comment, refactor u16 reading

Co-authored-by: Paul Masurel <paul@quickwit.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants