Skip to content

Commit

Permalink
Remove custom allocator support
Browse files Browse the repository at this point in the history 
The allocator support feature currently suffers from several issues.

1. It misuses the GlobalAlloc trait as opposed to the Allocator api to remain stable. The allocator API would be the best fit but it is currently unstable.

2. Secondly it uses a double Box to pass the GlobalAlloc trait object, because trait objects are fat pointers and across FFI boundaries for the brotli API only thin pointers are accepted. This would be more efficiently done using a ThinBox, which would only allocate once instead of twice.

3. Performance improvements are negligible

I plan to readd support later under a feature gate. Preferably once the required primitives are stabilized.
  • Loading branch information
@AronParker
AronParker committed Aug 14, 2022
1 parent 505314a commit e898e20
Show file tree
Hide file tree
Showing 4 changed files with 4 additions and 341 deletions.
88 changes: 1 addition & 87 deletions src/decode.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
//! [`Read`]: https://doc.rust-lang.org/stable/std/io/trait.Read.html
//! [`Write`]: https://doc.rust-lang.org/stable/std/io/trait.Write.html
use std::alloc::GlobalAlloc;
use std::error::Error;
use std::ffi::CStr;
use std::io::{BufRead, Read, Write};
Expand All @@ -24,10 +23,6 @@ use crate::{IntoInnerError, SetParameterError};
/// [`DecompressorWriter`].
pub struct BrotliDecoder {
state: *mut BrotliDecoderState,

// this field is read read across FFI boundaries
#[allow(dead_code)]
alloc: Option<Box<Box<dyn GlobalAlloc>>>,
}

unsafe impl Send for BrotliDecoder {}
Expand All @@ -44,36 +39,7 @@ impl BrotliDecoder {
let instance = unsafe { BrotliDecoderCreateInstance(None, None, ptr::null_mut()) };

if !instance.is_null() {
BrotliDecoder {
state: instance,
alloc: None,
}
} else {
panic!("BrotliDecoderCreateInstance returned NULL: failed to allocate or initialize");
}
}

/// Constructs a new brotli decoder instance using allocator `alloc`.
///
/// # Panics
///
/// Panics if the decoder fails to be allocated or initialized
#[doc(alias = "BrotliDecoderCreateInstance")]
pub fn new_in<A>(alloc: A) -> Self
where
A: GlobalAlloc + 'static,
{
let alloc: Box<Box<dyn GlobalAlloc>> = Box::new(Box::new(alloc));
let alloc_ptr: *const Box<dyn GlobalAlloc> = alloc.as_ref();
let instance = unsafe {
BrotliDecoderCreateInstance(Some(crate::malloc), Some(crate::free), alloc_ptr as _)
};

if !instance.is_null() {
BrotliDecoder {
state: instance,
alloc: Some(alloc),
}
BrotliDecoder { state: instance }
} else {
panic!("BrotliDecoderCreateInstance returned NULL: failed to allocate or initialize");
}
Expand Down Expand Up @@ -379,25 +345,6 @@ impl BrotliDecoderOptions {
Ok(decoder)
}

/// Creates a brotli decoder with the specified settings using allocator
/// `alloc`.
///
/// # Errors
///
/// If any of the preconditions of the parameters are violated, an error is
/// returned.
#[doc(alias = "BrotliDecoderSetParameter")]
pub fn build_in<A>(&self, alloc: A) -> Result<BrotliDecoder, SetParameterError>
where
A: GlobalAlloc + 'static,
{
let mut decoder = BrotliDecoder::new_in(alloc);

self.configure(&mut decoder)?;

Ok(decoder)
}

fn configure(&self, decoder: &mut BrotliDecoder) -> Result<(), SetParameterError> {
if let Some(disable_ring_buffer_reallocation) = self.disable_ring_buffer_reallocation {
let key = BrotliDecoderParameter_BROTLI_DECODER_PARAM_DISABLE_RING_BUFFER_REALLOCATION;
Expand Down Expand Up @@ -555,22 +502,6 @@ impl<R: BufRead> DecompressorReader<R> {
}
}

/// Creates a new `DecompressorReader<R>` with a newly created decoder using
/// allocator `alloc`.
///
/// # Panics
///
/// Panics if the decoder fails to be allocated or initialized
pub fn new_in<A>(inner: R, alloc: A) -> Self
where
A: GlobalAlloc + 'static,
{
DecompressorReader {
inner,
decoder: BrotliDecoder::new_in(alloc),
}
}

/// Creates a new `DecompressorReader<R>` with a specified decoder.
///
/// # Examples
Expand Down Expand Up @@ -705,23 +636,6 @@ impl<W: Write> DecompressorWriter<W> {
}
}

/// Creates a new `DecompressorWriter<W>` with a newly created decoder using
/// allocator `alloc`.
///
/// # Panics
///
/// Panics if the decoder fails to be allocated or initialized
pub fn new_in<A>(inner: W, alloc: A) -> Self
where
A: GlobalAlloc + 'static,
{
DecompressorWriter {
inner,
decoder: BrotliDecoder::new_in(alloc),
panicked: false,
}
}

/// Creates a new `DecompressorWriter<W>` with a specified decoder.
///
/// # Examples
Expand Down
88 changes: 1 addition & 87 deletions src/encode.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
//! [`Read`]: https://doc.rust-lang.org/stable/std/io/trait.Read.html
//! [`Write`]: https://doc.rust-lang.org/stable/std/io/trait.Write.html
use std::alloc::GlobalAlloc;
use std::error::Error;
use std::io::{BufRead, Read, Write};
use std::{fmt, io, mem, ptr, slice};
Expand All @@ -26,10 +25,6 @@ use crate::{
/// [`CompressorWriter`].
pub struct BrotliEncoder {
state: *mut BrotliEncoderState,

// this field is read read across FFI boundaries
#[allow(dead_code)]
alloc: Option<Box<Box<dyn GlobalAlloc>>>,
}

unsafe impl Send for BrotliEncoder {}
Expand All @@ -46,36 +41,7 @@ impl BrotliEncoder {
let instance = unsafe { BrotliEncoderCreateInstance(None, None, ptr::null_mut()) };

if !instance.is_null() {
BrotliEncoder {
state: instance,
alloc: None,
}
} else {
panic!("BrotliEncoderCreateInstance returned NULL: failed to allocate or initialize");
}
}

/// Constructs a new brotli encoder instance using allocator `alloc`.
///
/// # Panics
///
/// Panics if the encoder fails to be allocated or initialized
#[doc(alias = "BrotliEncoderCreateInstance")]
pub fn new_in<A>(alloc: A) -> Self
where
A: GlobalAlloc + 'static,
{
let alloc: Box<Box<dyn GlobalAlloc>> = Box::new(Box::new(alloc));
let alloc_ptr: *const Box<dyn GlobalAlloc> = alloc.as_ref();
let instance = unsafe {
BrotliEncoderCreateInstance(Some(crate::malloc), Some(crate::free), alloc_ptr as _)
};

if !instance.is_null() {
BrotliEncoder {
state: instance,
alloc: Some(alloc),
}
BrotliEncoder { state: instance }
} else {
panic!("BrotliEncoderCreateInstance returned NULL: failed to allocate or initialize");
}
Expand Down Expand Up @@ -445,24 +411,6 @@ impl BrotliEncoderOptions {
Ok(encoder)
}

/// Creates a brotli encoder using the specified settings.
///
/// # Errors
///
/// If any of the preconditions of the parameters are violated, an error is
/// returned.
#[doc(alias = "BrotliEncoderSetParameter")]
pub fn build_in<A>(&self, alloc: A) -> Result<BrotliEncoder, SetParameterError>
where
A: GlobalAlloc + 'static,
{
let mut encoder = BrotliEncoder::new_in(alloc);

self.configure(&mut encoder)?;

Ok(encoder)
}

fn configure(&self, encoder: &mut BrotliEncoder) -> Result<(), SetParameterError> {
if let Some(mode) = self.mode {
let key = BrotliEncoderParameter_BROTLI_PARAM_MODE;
Expand Down Expand Up @@ -638,23 +586,6 @@ impl<R: BufRead> CompressorReader<R> {
}
}

/// Creates a new `CompressorReader<R>` with a newly created encoder using
/// allocator `alloc`.
///
/// # Panics
///
/// Panics if the encoder fails to be allocated or initialized
pub fn new_in<A>(inner: R, alloc: A) -> Self
where
A: GlobalAlloc + 'static,
{
CompressorReader {
inner,
encoder: BrotliEncoder::new_in(alloc),
op: BrotliOperation::Process,
}
}

/// Creates a new `CompressorReader<R>` with a specified encoder.
///
/// # Examples
Expand Down Expand Up @@ -803,23 +734,6 @@ impl<W: Write> CompressorWriter<W> {
}
}

/// Creates a new `CompressorWriter<W>` with a newly created encoder using
/// allocator `alloc`.
///
/// # Panics
///
/// Panics if the encoder fails to be allocated or initialized
pub fn new_in<A>(inner: W, alloc: A) -> Self
where
A: GlobalAlloc + 'static,
{
CompressorWriter {
inner,
encoder: BrotliEncoder::new_in(alloc),
panicked: false,
}
}

/// Creates a new `CompressorWriter<W>` with a specified encoder.
///
/// # Examples
Expand Down
39 changes: 2 additions & 37 deletions src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,10 +125,9 @@
pub mod decode;
pub mod encode;

use std::alloc::{GlobalAlloc, Layout};
use std::error::Error;
use std::os::raw::{c_int, c_void};
use std::{fmt, io, ptr};
use std::os::raw::c_int;
use std::{fmt, io};

use brotlic_sys::*;
pub use decode::{BrotliDecoder, BrotliDecoderOptions, DecompressorReader, DecompressorWriter};
Expand Down Expand Up @@ -1027,37 +1026,3 @@ impl<I> fmt::Display for IntoInnerError<I> {
self.error().fmt(f)
}
}

const MIN_ALIGN: usize = 16;

extern "C" fn malloc(opaque: *mut c_void, size: usize) -> *mut c_void {
let global_alloc = opaque as *const Box<dyn GlobalAlloc>;

if size > usize::MAX - 2 * MIN_ALIGN + 1 {
return ptr::null_mut();
}

unsafe {
let layout = Layout::from_size_align_unchecked(size + MIN_ALIGN, MIN_ALIGN);
let alloc = (*global_alloc).alloc(layout);
(alloc as *mut usize).write(size);

(alloc.add(MIN_ALIGN)) as _
}
}

extern "C" fn free(opaque: *mut c_void, address: *mut c_void) {
if address.is_null() {
return;
}

let global_alloc = opaque as *const Box<dyn GlobalAlloc>;

unsafe {
let alloc = (address as *mut u8).sub(MIN_ALIGN);
let size = (alloc as *const usize).read();
let layout = Layout::from_size_align_unchecked(size + MIN_ALIGN, MIN_ALIGN);

(*global_alloc).dealloc(alloc, layout);
}
}
Loading

0 comments on commit e898e20

Please sign in to comment.