Skip to content

Commit

Permalink
Don't pointlessly block on glyph order
Browse files Browse the repository at this point in the history
  • Loading branch information
@rsheeter
rsheeter committed Nov 11, 2023
1 parent 0a00caa commit c4f45ff
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 31 deletions.
4 changes: 2 additions & 2 deletions fontc/src/error.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,6 @@ pub enum Error {
TasksFailed(Vec<(AnyWorkId, String)>),
#[error("Invalid regex")]
BadRegex(#[from] regex::Error),
#[error("Unable to proceed")]
UnableToProceed,
#[error("Unable to proceed; {0} jobs stuck pending")]
UnableToProceed(usize),
}
80 changes: 51 additions & 29 deletions fontc/src/workload.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use fontir::{
orchestration::{Context as FeContext, WorkId as FeWorkIdentifier},
source::Input,
};
use log::{debug, trace};
use log::{debug, trace, warn};

use crate::{
timing::{create_timer, JobTime, JobTimer},
Expand Down Expand Up @@ -134,7 +134,16 @@ impl<'a> Workload<'a> {
}
}

fn update_be_glyph_dependencies(&mut self, fe_root: &FeContext, glyph_name: GlyphName) {
/// 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 can refine that:
///
/// * If the glyph doesn't emit to binary we don't need to do the BE work at all
/// * If the glyph has no components the BE for it doesn't use glyph order and needn't block on it
/// * If the glyph does have components we need to block on glyph order because that might alter them
/// * For example, flatten
///
/// By minimizing dependencies we allow jobs to start earlier and execute with greater concurrency.
fn update_be_glyph_work(&mut self, fe_root: &FeContext, glyph_name: GlyphName) {
let glyph = fe_root
.glyphs
.get(&FeWorkIdentifier::Glyph(glyph_name.clone()));
Expand All @@ -144,17 +153,28 @@ impl<'a> Workload<'a> {
.get_mut(&be_id)
.expect("{be_id} should exist");

let mut deps = HashSet::from([
AnyWorkId::Fe(FeWorkIdentifier::StaticMetadata),
FeWorkIdentifier::GlyphOrder.into(),
]);
if !glyph.emit_to_binary {
trace!("Skipping execution of {be_id:?}; it does not emit to binary");
self.jobs_pending.remove(&be_id);
self.mark_also_completed(&be_id);
self.success.insert(be_id);
return;
}

let mut deps = HashSet::from([AnyWorkId::Fe(FeWorkIdentifier::StaticMetadata)]);

let mut has_components = false;
for inst in glyph.sources().values() {
for component in inst.components.iter() {
has_components = true;
deps.insert(FeWorkIdentifier::Glyph(component.base.clone()).into());
}
}

if has_components {
deps.insert(FeWorkIdentifier::GlyphOrder.into());
}

trace!(
"Updating {be_id:?} deps from {:?} to {deps:?}",
be_job.read_access
Expand All @@ -181,16 +201,12 @@ impl<'a> Workload<'a> {
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());
self.update_be_glyph_work(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);
self.update_be_glyph_work(fe_root, glyph_name);
}
}

Expand Down Expand Up @@ -232,30 +248,30 @@ impl<'a> Workload<'a> {
.queued()
.run();

let mut has_kern = false;
let mut move_to_front = Vec::new();
let mut launchable: Vec<_> = self
.jobs_pending
.iter()
.filter_map(|(id, job)| {
if !job.running && self.can_run(job) {
if matches!(id, AnyWorkId::Fe(FeWorkIdentifier::Kerning)) {
has_kern = true;
}
Some(id.clone())
} else {
None
.filter_map(|(id, job)| (!job.running && self.can_run(job)).then(|| id))
.enumerate()
.map(|(idx, id)| {
if matches!(
id,
AnyWorkId::Fe(FeWorkIdentifier::Kerning)
| AnyWorkId::Fe(FeWorkIdentifier::GlyphOrder)
) {
move_to_front.push(idx);
}
id
})
.cloned()
.collect();
trace!("Launchable: {launchable:?}");

// https://github.com/googlefonts/fontc/issues/456: try to avoid kern as long pole
if has_kern {
let kern_idx = launchable
.iter()
.position(|id| matches!(id, AnyWorkId::Fe(FeWorkIdentifier::Kerning)))
.unwrap();
launchable.swap(0, kern_idx);
// Try to prioritize the critical path
// <https://github.com/googlefonts/fontc/issues/456>, <https://github.com/googlefonts/fontc/pull/565>
for (idx, move_idx) in move_to_front.into_iter().enumerate() {
launchable.swap(idx, move_idx);
}

self.timer.add(timing.complete());
Expand All @@ -279,7 +295,13 @@ impl<'a> Workload<'a> {
// Spawn anything that is currently executable (has no unfulfilled dependencies)
let launchable = self.launchable();
if launchable.is_empty() && !self.jobs_pending.values().any(|j| j.running) {
return Err(Error::UnableToProceed);
if log::log_enabled!(log::Level::Warn) {
warn!("{}/{} jobs have succeeded, nothing is running, and nothing is launchable", self.success.len(), self.job_count);
for pending in self.jobs_pending.keys() {
warn!(" blocked: {pending:?}");
}
}
return Err(Error::UnableToProceed(self.jobs_pending.len()));
}

// Launch anything that needs launching
Expand Down

0 comments on commit c4f45ff

Please sign in to comment.