Skip to content

Add method to convert non-shared Collection/SourceCache to shared variants#541

Open
nicoburns wants to merge 3 commits intolinebender:mainfrom
nicoburns:make-shared
Open

Add method to convert non-shared Collection/SourceCache to shared variants#541
nicoburns wants to merge 3 commits intolinebender:mainfrom
nicoburns:make-shared

Conversation

@nicoburns
Copy link
Collaborator

@nicoburns nicoburns commented Feb 6, 2026

This PR allows a Collection or SourceCache that was constructed as non-shared, to be later converted to be shared. This is useful when accepting FontContext/Collection/SourceCache provided by users (which may or may not be shared - as decided by the user) which you then want to "fan out" into thread-local shared clones.

Blitz is using the new make_shared method on SourceCache introduced in this PR. Blitz is not currently using the new corresponding make_shared method on Collection, but that method is included in this PR anyway for symmetry.

@nicoburns nicoburns force-pushed the make-shared branch 3 times, most recently from 5795bf4 to 3bbf724 Compare February 6, 2026 21:52
@nicoburns nicoburns requested a review from dfrg February 9, 2026 14:21
@nicoburns nicoburns requested a review from taj-p March 5, 2026 13:36
pub fn make_shared(&mut self) {
if self.shared.is_none() {
self.shared = Some(Arc::new(Shared {
data: Mutex::new(core::mem::take(&mut self.data)),
Copy link
Contributor

@taj-p taj-p Mar 10, 2026

Choose a reason for hiding this comment

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

I think there is a bug here. Do we need to increment the self.shared_version too or clone instead of take? Otherwise, local data seems to be lost on subsequent reads.

For example, this example code fails on the assert because all our reads are guarded through sync_shared. Since the version hasn't changed, we don't update our self.data.

use parley::fontique::{Collection, CollectionOptions};

fn main() {
    let mut collection = Collection::new(CollectionOptions {
        shared: false,
        system_fonts: false,
    });

    let font_dirs: Vec<std::path::PathBuf> = ["/System/Library/Fonts", "/Library/Fonts"]
        .iter()
        .map(std::path::PathBuf::from)
        .filter(|p| p.is_dir())
        .collect();

    if font_dirs.is_empty() {
        eprintln!("No system font directories found — cannot demonstrate the bug.");
        std::process::exit(1);
    }

    collection.load_fonts_from_paths(&font_dirs);

    let names_before: Vec<String> = collection.family_names().map(String::from).collect();

    collection.make_shared();

    let names_after: Vec<String> = collection.family_names().map(String::from).collect();

    assert_eq!(names_before.len(), names_after.len());
}
Image

Signed-off-by: Nico Burns <nico@nicoburns.com>
Signed-off-by: Nico Burns <nico@nicoburns.com>
@nicoburns nicoburns requested a review from taj-p March 12, 2026 19:07
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.

2 participants