Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

prevent unneeded offset check #1059

Merged
merged 3 commits into from
Jun 8, 2022

Conversation

ritchie46
Copy link
Collaborator

@ritchie46 ritchie46 commented Jun 8, 2022

Calling ListArray::new does O(n) work where n = offset_length.

However, this work can be prevented in a few cases because we convert from a struct that has the same invariants.

The same logic applies to a Utf8 conversion, where we just determined the offsets and converted &str to bytes, so a utf8 check is redundant.

@ritchie46
Copy link
Collaborator Author

ritchie46 commented Jun 8, 2022

Hmm.. it fails on forbid_unsafe in avro/json. I can remove it there. I can also add an extra debug check on the invariants.

@ritchie46
Copy link
Collaborator Author

We could actually abstract these unsafe calls by having two newtype structs

#[repr(transparent)]
struct OffsetBuffer<O: Offset>(Vec<O>)

impl OffsetBuffer {

  fn add_lenght(&mut self, len: usize);
  
  unsafe fn from_vec();
}

// this could also be just `String`
// but maybe this is easier with delaying Utf8 validation
#[repr(transparent)]
struct StringBuffer(Vec<u8>)

impl StringBuffer {

   fn push_str(&mut self, val: &str);
   
   unsafe fn push_bytes(&mut self, bytes: &[u8]);
   
   unsafe fn from_vec();
}

A MutableList can then call new and pass on its internal state with the given invariants. That way you can keep the io module unsafe free without doing redundant checks.

@codecov
Copy link

codecov bot commented Jun 8, 2022

Codecov Report

Merging #1059 (49e5c82) into main (86e533f) will increase coverage by 0.06%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main    #1059      +/-   ##
==========================================
+ Coverage   81.25%   81.32%   +0.06%     
==========================================
  Files         363      363              
  Lines       34673    34685      +12     
==========================================
+ Hits        28172    28206      +34     
+ Misses       6501     6479      -22     
Impacted Files Coverage Δ
src/array/list/mutable.rs 75.44% <66.66%> (-0.68%) ⬇️
src/io/ipc/read/stream_async.rs 74.38% <0.00%> (+0.82%) ⬆️
src/array/list/mod.rs 84.81% <0.00%> (+13.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86e533f...49e5c82. Read the comment docs.

@jorgecarleitao jorgecarleitao merged commit b0c601e into jorgecarleitao:main Jun 8, 2022
@jorgecarleitao
Copy link
Owner

Great idea - we can offer a safe API around those!

@jorgecarleitao jorgecarleitao added the enhancement An improvement to an existing feature label Jun 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement An improvement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants