From 2a81dbf44803873e85c99d7becc9750c98787795 Mon Sep 17 00:00:00 2001 From: Rod S Date: Tue, 8 Aug 2023 09:37:55 -0300 Subject: [PATCH] Restore fine-grained glyph work dependencies --- fontbe/src/glyphs.rs | 14 +++++----- fontc/src/workload.rs | 45 +++++++++++++++++++++++++++++++-- fontdrasil/src/orchestration.rs | 7 ++++- 3 files changed, 55 insertions(+), 11 deletions(-) diff --git a/fontbe/src/glyphs.rs b/fontbe/src/glyphs.rs index d4649cec..364b154f 100644 --- a/fontbe/src/glyphs.rs +++ b/fontbe/src/glyphs.rs @@ -240,15 +240,13 @@ impl Work for GlyphWork { WorkId::GlyfFragment(self.glyph_name.clone()).into() } + /// We need to block on all our components, but we don't know them yet. + /// + /// We could block on ALL IR glyphs, but that triggers inefficient behavior in workload.rs. + /// Instead, start in a hard block and update upon success of the corresponding IR job. + /// See fontc, workload.rs, handle_success. fn read_access(&self) -> Access { - Access::Custom(Arc::new(|id| { - matches!( - id, - AnyWorkId::Fe(FeWorkId::StaticMetadata) - | AnyWorkId::Fe(FeWorkId::GlyphOrder) - | AnyWorkId::Fe(FeWorkId::Glyph(..)) - ) - })) + Access::Unknown } fn write_access(&self) -> Access { diff --git a/fontc/src/workload.rs b/fontc/src/workload.rs index f5055687..f5debcd9 100644 --- a/fontc/src/workload.rs +++ b/fontc/src/workload.rs @@ -11,8 +11,8 @@ use std::{ }; use crossbeam_channel::{Receiver, TryRecvError}; -use fontbe::orchestration::{AnyWorkId, Context as BeContext}; -use fontdrasil::orchestration::Access; +use fontbe::orchestration::{AnyWorkId, Context as BeContext, WorkId as BeWorkIdentifier}; +use fontdrasil::{orchestration::Access, types::GlyphName}; use fontir::{ orchestration::{Context as FeContext, WorkId as FeWorkIdentifier}, source::Input, @@ -129,6 +129,34 @@ impl<'a> Workload<'a> { } } + fn update_be_glyph_dependencies(&mut self, fe_root: &FeContext, glyph_name: GlyphName) { + let glyph = fe_root + .glyphs + .get(&FeWorkIdentifier::Glyph(glyph_name.clone())); + let be_id = AnyWorkId::Be(BeWorkIdentifier::GlyfFragment(glyph_name)); + let be_job = self + .jobs_pending + .get_mut(&be_id) + .expect("{be_id} should exist"); + + let mut deps = HashSet::from([ + AnyWorkId::Fe(FeWorkIdentifier::StaticMetadata), + FeWorkIdentifier::GlyphOrder.into(), + ]); + + for inst in glyph.sources().values() { + for component in inst.components.iter() { + deps.insert(FeWorkIdentifier::Glyph(component.base.clone()).into()); + } + } + + trace!( + "Updating {be_id:?} deps from {:?} to {deps:?}", + be_job.read_access + ); + be_job.read_access = Access::Set(deps).into(); + } + fn handle_success(&mut self, fe_root: &FeContext, success: AnyWorkId) { log::debug!("{success:?} successful"); @@ -144,8 +172,19 @@ impl<'a> Workload<'a> { { debug!("Generating a BE job for {glyph_name}"); super::add_glyph_be_job(self, glyph_name.clone()); + + // Glyph order is done so all IR must be done. Copy dependencies from the IR for the same name. + self.update_be_glyph_dependencies(fe_root, glyph_name.clone()); } } + + // When BE glyph jobs are initially created they don't know enough to set fine grained dependencies + // so they depend on *all* IR glyphs. Once IR for a glyph completes we know enough to refine that + // to just the glyphs our glyphs uses as components. This allows BE glyph jobs to run concurrently with + // unrelated IR jobs. + if let AnyWorkId::Fe(FeWorkIdentifier::Glyph(glyph_name)) = success { + self.update_be_glyph_dependencies(fe_root, glyph_name); + } } fn can_run_scan(&self, job: &Job) -> bool { @@ -162,6 +201,7 @@ impl<'a> Workload<'a> { match &job.read_access { AnyAccess::Fe(access) => match access { Access::None => true, + Access::Unknown => false, Access::One(id) => !self.jobs_pending.contains_key(&id.clone().into()), Access::Set(ids) => !ids .iter() @@ -171,6 +211,7 @@ impl<'a> Workload<'a> { }, AnyAccess::Be(access) => match access { Access::None => true, + Access::Unknown => false, Access::One(id) => !self.jobs_pending.contains_key(id), Access::Set(ids) => !ids.iter().any(|id| self.jobs_pending.contains_key(id)), Access::Custom(..) => self.can_run_scan(job), diff --git a/fontdrasil/src/orchestration.rs b/fontdrasil/src/orchestration.rs index b62730c9..83f112fa 100644 --- a/fontdrasil/src/orchestration.rs +++ b/fontdrasil/src/orchestration.rs @@ -17,8 +17,11 @@ pub trait Identifier: Debug + Clone + Eq + Hash {} /// A rule that represents whether access to something is permitted. #[derive(Clone)] pub enum Access { - /// No access is permitted + /// Nothing is accessed None, + /// Access requirements not yet known. Intended to be used when what access is needed + /// isn't initially known. Once known, access will change to some other option. + Unknown, /// Any access is permitted All, /// Access to one specific resource is permitted @@ -33,6 +36,7 @@ impl Debug for Access { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { Self::None => write!(f, "None"), + Self::Unknown => write!(f, "Unknown"), Self::All => write!(f, "All"), Self::One(arg0) => f.debug_tuple("One").field(arg0).finish(), Self::Set(arg0) => f.debug_tuple("Set").field(arg0).finish(), @@ -154,6 +158,7 @@ impl Access { pub fn check(&self, id: &I) -> bool { match self { Access::None => false, + Access::Unknown => false, Access::All => true, Access::One(allow) => id == allow, Access::Set(ids) => ids.contains(id),