Skip to content

Transform Rc usage to Arc for multithreading #122

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

Transform Rc usage to Arc for multithreading #122

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a178bbb
Transform Rc usage to Arc for multithreading
fschutt Sep 12, 2025
1a98d78
Add from_owned for KernTable
fschutt Sep 12, 2025
1ddc1c1
RefCell -> Mutex
fschutt Sep 12, 2025
399999d
I have replaced `std::sync::Mutex` with `parking_lot::ReentrantMutex`…
google-labs-jules[bot] Sep 16, 2025
eee1538
Merge pull request #2 from fschutt/resolve-deadlock
fschutt Sep 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions src/binary/read.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use std::collections::hash_map::Entry;
use std::collections::HashMap;
use std::fmt;
use std::marker::PhantomData;
use std::rc::Rc;
use std::sync::Arc;

#[derive(Debug, Copy, Clone)]
pub struct ReadEof {}
Expand Down Expand Up @@ -60,7 +60,7 @@ pub struct ReadCtxt<'a> {
}

pub struct ReadCache<T> {
map: HashMap<usize, Rc<T>>,
map: HashMap<usize, Arc<T>>,
}

pub trait ReadBinary {
Expand Down Expand Up @@ -306,34 +306,34 @@ impl<'a> ReadScope<'a> {
pub fn read_cache<T>(
&self,
cache: &mut ReadCache<T::HostType<'a>>,
) -> Result<Rc<T::HostType<'a>>, ParseError>
) -> Result<Arc<T::HostType<'a>>, ParseError>
where
T: 'static + ReadBinaryDep<Args<'a> = ()>,
{
match cache.map.entry(self.base) {
Entry::Vacant(entry) => {
let t = Rc::new(self.read::<T>()?);
Ok(Rc::clone(entry.insert(t)))
let t = Arc::new(self.read::<T>()?);
Ok(Arc::clone(entry.insert(t)))
}
Entry::Occupied(entry) => Ok(Rc::clone(entry.get())),
Entry::Occupied(entry) => Ok(Arc::clone(entry.get())),
}
}

pub fn read_cache_state<T, Table>(
&self,
cache: &mut ReadCache<T::HostType<'a>>,
state: LayoutCache<Table>,
) -> Result<Rc<T::HostType<'a>>, ParseError>
) -> Result<Arc<T::HostType<'a>>, ParseError>
where
T: 'static + ReadBinaryDep<Args<'a> = LayoutCache<Table>>,
Table: LayoutTableType,
{
match cache.map.entry(self.base) {
Entry::Vacant(entry) => {
let t = Rc::new(self.read_dep::<T>(state)?);
Ok(Rc::clone(entry.insert(t)))
let t = Arc::new(self.read_dep::<T>(state)?);
Ok(Arc::clone(entry.insert(t)))
}
Entry::Occupied(entry) => Ok(Rc::clone(entry.get())),
Entry::Occupied(entry) => Ok(Arc::clone(entry.get())),
}
}

Expand Down
6 changes: 3 additions & 3 deletions src/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

use std::marker::PhantomData;
use std::ops::RangeInclusive;
use std::rc::Rc;
use std::sync::Arc;

use crate::gdef;
use crate::layout::{ClassDef, Coverage, GDEFTable};
Expand All @@ -28,8 +28,8 @@ pub struct MatchType {
pub enum GlyphTable<'a> {
Empty,
ById(&'a [u16]),
ByClassDef(Rc<ClassDef>, &'a [u16]),
ByCoverage(&'a [Rc<Coverage>]),
ByClassDef(Arc<ClassDef>, &'a [u16]),
ByCoverage(&'a [Arc<Coverage>]),
}

impl GlyphTable<'_> {
Expand Down
54 changes: 27 additions & 27 deletions src/font.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

use std::borrow::Cow;
use std::convert::{self};
use std::rc::Rc;
use std::sync::Arc;

use bitflags::bitflags;
use pathfinder_geometry::rect::RectF;
Expand Down Expand Up @@ -89,23 +89,23 @@ pub struct Font<T: FontTableProvider> {
pub maxp_table: MaxpTable,
hmtx_table: Box<[u8]>,
pub hhea_table: HheaTable,
vmtx_table: LazyLoad<Rc<[u8]>>,
vhea_table: LazyLoad<Rc<HheaTable>>,
vmtx_table: LazyLoad<Arc<[u8]>>,
vhea_table: LazyLoad<Arc<HheaTable>>,
cmap_subtable_offset: usize,
pub cmap_subtable_encoding: Encoding,
gdef_cache: LazyLoad<Rc<GDEFTable>>,
morx_cache: LazyLoad<Rc<tables::Morx>>,
gdef_cache: LazyLoad<Arc<GDEFTable>>,
morx_cache: LazyLoad<Arc<tables::Morx>>,
gsub_cache: LazyLoad<LayoutCache<GSUB>>,
gpos_cache: LazyLoad<LayoutCache<GPOS>>,
kern_cache: LazyLoad<Rc<KernTable>>,
kern_cache: LazyLoad<Arc<KernTable>>,
os2_us_first_char_index: LazyLoad<u16>,
glyph_cache: GlyphCache,
pub glyph_table_flags: GlyphTableFlags,
loca_glyf: LocaGlyf,
cff_cache: LazyLoad<Rc<tables::CFF>>,
cff2_cache: LazyLoad<Rc<tables::CFF2>>,
cff_cache: LazyLoad<Arc<tables::CFF>>,
cff2_cache: LazyLoad<Arc<tables::CFF2>>,
embedded_image_filter: GlyphTableFlags,
embedded_images: LazyLoad<Rc<Images>>,
embedded_images: LazyLoad<Arc<Images>>,
axis_count: u16,
}

Expand Down Expand Up @@ -403,7 +403,7 @@ impl<T: FontTableProvider> Font<T> {
let opt_gpos_cache = check_set_err(self.gpos_cache(), &mut err);
let opt_gdef_table = check_set_err(self.gdef_table(), &mut err);
let opt_morx_table = check_set_err(self.morx_table(), &mut err);
let opt_gdef_table = opt_gdef_table.as_ref().map(Rc::as_ref);
let opt_gdef_table = opt_gdef_table.as_ref().map(Arc::as_ref);
let opt_kern_table = check_set_err(self.kern_table(), &mut err);
let opt_kern_table = opt_kern_table
.as_ref()
Expand Down Expand Up @@ -771,7 +771,7 @@ impl<T: FontTableProvider> Font<T> {
table_builder: |data: &Box<[u8]>| ReadScope::new(data).read::<CFF2<'_>>(),
}
.try_build()?;
Ok(Some(Rc::new(cff)))
Ok(Some(Arc::new(cff)))
})?
.ok_or(ParseError::MissingTable(tag::CFF2))?;

Expand All @@ -796,7 +796,7 @@ impl<T: FontTableProvider> Font<T> {
table_builder: |data: &Box<[u8]>| ReadScope::new(data).read::<CFF<'_>>(),
}
.try_build()?;
Ok(Some(Rc::new(cff)))
Ok(Some(Arc::new(cff)))
})?
.ok_or(ParseError::MissingTable(tag::CFF))?;

Expand Down Expand Up @@ -978,24 +978,24 @@ impl<T: FontTableProvider> Font<T> {
)
}

fn embedded_images(&mut self) -> Result<Option<Rc<Images>>, ParseError> {
fn embedded_images(&mut self) -> Result<Option<Arc<Images>>, ParseError> {
let provider = &self.font_table_provider;
let num_glyphs = usize::from(self.maxp_table.num_glyphs);
let tables_to_check = self.glyph_table_flags & self.embedded_image_filter;
self.embedded_images.get_or_load(|| {
if tables_to_check.contains(GlyphTableFlags::COLR) {
let images = load_colr_cpal(provider).map(Images::Colr)?;
Ok(Some(Rc::new(images)))
Ok(Some(Arc::new(images)))
} else if tables_to_check.contains(GlyphTableFlags::SVG) {
let images = load_svg(provider).map(Images::Svg)?;
Ok(Some(Rc::new(images)))
Ok(Some(Arc::new(images)))
} else if tables_to_check.contains(GlyphTableFlags::CBDT) {
let images = load_cblc_cbdt(provider, tag::CBLC, tag::CBDT)
.map(|(cblc, cbdt)| Images::Embedded { cblc, cbdt })?;
Ok(Some(Rc::new(images)))
Ok(Some(Arc::new(images)))
} else if tables_to_check.contains(GlyphTableFlags::SBIX) {
let images = load_sbix(provider, num_glyphs).map(Images::Sbix)?;
Ok(Some(Rc::new(images)))
Ok(Some(Arc::new(images)))
} else if tables_to_check.contains(GlyphTableFlags::EBDT) {
let images =
load_cblc_cbdt(provider, tag::EBLC, tag::EBDT).map(|(eblc, ebdt)| {
Expand All @@ -1004,7 +1004,7 @@ impl<T: FontTableProvider> Font<T> {
cbdt: ebdt,
}
})?;
Ok(Some(Rc::new(images)))
Ok(Some(Arc::new(images)))
} else {
Ok(None)
}
Expand Down Expand Up @@ -1040,7 +1040,7 @@ impl<T: FontTableProvider> Font<T> {
let vmtx = self
.vmtx_table
.get_or_load(|| {
read_and_box_optional_table(provider, tag::VMTX).map(|ok| ok.map(Rc::from))
read_and_box_optional_table(provider, tag::VMTX).map(|ok| ok.map(Arc::from))
})
.ok()?;
let vhea = self.vhea_table().ok()?;
Expand All @@ -1056,27 +1056,27 @@ impl<T: FontTableProvider> Font<T> {
load_os2_table(&self.font_table_provider)
}

pub fn gdef_table(&mut self) -> Result<Option<Rc<GDEFTable>>, ParseError> {
pub fn gdef_table(&mut self) -> Result<Option<Arc<GDEFTable>>, ParseError> {
let provider = &self.font_table_provider;
self.gdef_cache.get_or_load(|| {
if let Some(gdef_data) = provider.table_data(tag::GDEF)? {
let gdef = ReadScope::new(&gdef_data).read::<GDEFTable>()?;
Ok(Some(Rc::new(gdef)))
Ok(Some(Arc::new(gdef)))
} else {
Ok(None)
}
})
}

pub fn morx_table(&mut self) -> Result<Option<Rc<tables::Morx>>, ParseError> {
pub fn morx_table(&mut self) -> Result<Option<Arc<tables::Morx>>, ParseError> {
let provider = &self.font_table_provider;
let num_glyphs = self.num_glyphs();
self.morx_cache.get_or_load(|| {
if let Some(morx_data) = provider.table_data(tag::MORX)? {
let morx = tables::Morx::try_new(morx_data.into(), |data| {
ReadScope::new(data).read_dep::<MorxTable<'_>>(num_glyphs)
})?;
Ok(Some(Rc::new(morx)))
Ok(Some(Arc::new(morx)))
} else {
Ok(None)
}
Expand Down Expand Up @@ -1109,12 +1109,12 @@ impl<T: FontTableProvider> Font<T> {
})
}

pub fn kern_table(&mut self) -> Result<Option<Rc<KernTable>>, ParseError> {
pub fn kern_table(&mut self) -> Result<Option<Arc<KernTable>>, ParseError> {
let provider = &self.font_table_provider;
self.kern_cache.get_or_load(|| {
if let Some(kern_data) = provider.table_data(tag::KERN)? {
match ReadScope::new(&kern_data).read::<kern::KernTable<'_>>() {
Ok(kern) => Ok(Some(Rc::new(kern.to_owned()))),
Ok(kern) => Ok(Some(Arc::new(kern.to_owned()))),
// This error may be encountered because there is a kern 1.0 version defined by
// Apple that is not in the OpenType spec. It only works on macOS. We don't
// support it so return None instead of returning an error.
Expand All @@ -1127,12 +1127,12 @@ impl<T: FontTableProvider> Font<T> {
})
}

pub fn vhea_table(&mut self) -> Result<Option<Rc<HheaTable>>, ParseError> {
pub fn vhea_table(&mut self) -> Result<Option<Arc<HheaTable>>, ParseError> {
let provider = &self.font_table_provider;
self.vhea_table.get_or_load(|| {
if let Some(vhea_data) = provider.table_data(tag::VHEA)? {
let vhea = ReadScope::new(&vhea_data).read::<HheaTable>()?;
Ok(Some(Rc::new(vhea)))
Ok(Some(Arc::new(vhea)))
} else {
Ok(None)
}
Expand Down
10 changes: 5 additions & 5 deletions src/glyph_info.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

use std::borrow::Cow;
use std::collections::HashMap;
use std::rc::Rc;
use std::sync::Arc;

use ouroboros::self_referencing;
use rustc_hash::FxHashMap;
Expand Down Expand Up @@ -127,16 +127,16 @@ fn unique_glyph_names<'a>(
let mut seen = FxHashMap::with_capacity_and_hasher(capacity, Default::default());
let mut unique_names = Vec::with_capacity(capacity);

for name in names.map(Rc::new) {
for name in names.map(Arc::new) {
let alt = *seen
.entry(Rc::clone(&name))
.entry(Arc::clone(&name))
.and_modify(|alt| *alt += 1)
.or_insert(0);
let unique_name = if alt == 0 {
name
} else {
// name is not unique, generate a new name for it
Rc::new(Cow::from(format!("{}.alt{:02}", name, alt)))
Arc::new(Cow::from(format!("{}.alt{:02}", name, alt)))
};

unique_names.push(unique_name)
Expand All @@ -147,7 +147,7 @@ fn unique_glyph_names<'a>(
// to name and it's been dropped.
unique_names
.into_iter()
.map(|name| Rc::try_unwrap(name).unwrap())
.map(|name| Arc::try_unwrap(name).unwrap())
.collect()
}

Expand Down
21 changes: 11 additions & 10 deletions src/gsub.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -916,7 +916,8 @@ fn get_supported_features(
) -> Result<FeatureMask, ParseError> {
let feature_mask = match gsub_cache
.supported_features
.borrow_mut()
.lock()
.unwrap()
.entry((script_tag, lang_tag_key(opt_lang_tag)))
{
Entry::Occupied(entry) => FeatureMask::from_bits_truncate(*entry.get()),
Expand Down Expand Up @@ -962,7 +963,7 @@ fn find_alternate(features_list: &[FeatureInfo], feature_tag: u32) -> Option<usi
///
/// ```
/// use std::error::Error;
/// use std::rc::Rc;
/// use std::sync::Arc;
///
/// use allsorts::binary::read::ReadScope;
/// use allsorts::error::ParseError;
Expand All @@ -988,7 +989,7 @@ fn find_alternate(features_list: &[FeatureInfo], feature_tag: u32) -> Option<usi
/// let opt_gsub_cache = font.gsub_cache()?;
/// let opt_gpos_cache = font.gpos_cache()?;
/// let opt_gdef_table = font.gdef_table()?;
/// let opt_gdef_table = opt_gdef_table.as_ref().map(Rc::as_ref);
/// let opt_gdef_table = opt_gdef_table.as_ref().map(Arc::as_ref);
///
/// // Map glyphs
/// //
Expand Down Expand Up @@ -1410,7 +1411,7 @@ pub fn get_lookups_cache_index(
feature_variations: Option<&FeatureTableSubstitution<'_>>,
feature_mask: FeatureMask,
) -> Result<usize, ParseError> {
let index = match gsub_cache.lookups_index.borrow_mut().entry((
let index = match gsub_cache.lookups_index.lock().unwrap().entry((
script_tag,
lang_tag_key(opt_lang_tag),
feature_mask.bits(),
Expand All @@ -1426,8 +1427,8 @@ pub fn get_lookups_cache_index(
feature_mask,
feature_variations,
)?;
let index = gsub_cache.cached_lookups.borrow().len();
gsub_cache.cached_lookups.borrow_mut().push(lookups);
let index = gsub_cache.cached_lookups.lock().unwrap().len();
gsub_cache.cached_lookups.lock().unwrap().push(lookups);
*entry.insert(index)
} else {
*entry.insert(0)
Expand Down Expand Up @@ -1552,8 +1553,8 @@ fn gsub_apply_default(
feature_variations,
feature_mask,
)?;
let lookups = &gsub_cache.cached_lookups.borrow()[index];
let lookups_frac = &gsub_cache.cached_lookups.borrow()[index_frac];
let lookups = &gsub_cache.cached_lookups.lock().unwrap()[index];
let lookups_frac = &gsub_cache.cached_lookups.lock().unwrap()[index_frac];
gsub_apply_lookups_frac(
gsub_cache,
gsub_table,
Expand All @@ -1570,7 +1571,7 @@ fn gsub_apply_default(
feature_variations,
feature_mask,
)?;
let lookups = &gsub_cache.cached_lookups.borrow()[index];
let lookups = &gsub_cache.cached_lookups.lock().unwrap()[index];
gsub_apply_lookups(gsub_cache, gsub_table, opt_gdef_table, lookups, glyphs)?;
}
}
Expand Down Expand Up @@ -1718,7 +1719,7 @@ fn apply_rvrn(
feature_variations,
FeatureMask::RVRN,
)?;
let lookups = &gsub_cache.cached_lookups.borrow()[index];
let lookups = &gsub_cache.cached_lookups.lock().unwrap()[index];
gsub_apply_lookups(gsub_cache, gsub_table, opt_gdef_table, lookups, glyphs)?;
Ok(())
}
Expand Down
Loading
Loading