Skip to content
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.

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

Restore fine-grained glyph work dependencies #380

Merged
merged 1 commit into from
Aug 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 6 additions & 8 deletions fontbe/src/glyphs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -240,15 +240,13 @@ impl Work<Context, AnyWorkId, Error> 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<AnyWorkId> {
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<AnyWorkId> {
Expand Down
45 changes: 43 additions & 2 deletions fontc/src/workload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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");

Expand All @@ -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 {
Expand All @@ -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()
Expand All @@ -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),
Expand Down
7 changes: 6 additions & 1 deletion fontdrasil/src/orchestration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<I: Identifier> {
/// 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
Expand All @@ -33,6 +36,7 @@ impl<I: Identifier> Debug for Access<I> {
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(),
Expand Down Expand Up @@ -154,6 +158,7 @@ impl<I: Identifier> Access<I> {
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),
Expand Down