Skip to content

Commit

Permalink
Code review
Browse files Browse the repository at this point in the history
  • Loading branch information
@rsheeter
rsheeter committed Mar 2, 2023
1 parent 3213e0f commit 15f146b
Show file tree
Hide file tree
Showing 7 changed files with 87 additions and 104 deletions.
4 changes: 2 additions & 2 deletions fontbe/src/features.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use fea_rs::{
parse::{SourceLoadError, SourceResolver},
Compiler, GlyphMap, GlyphName as FeaRsGlyphName,
};
use fontir::ir::Features;
use fontir::{ir::Features, orchestration::Flags};
use log::{debug, error, trace, warn};
use write_fonts::FontBuilder;

Expand Down Expand Up @@ -129,7 +129,7 @@ impl Work<Context, Error> for FeatureWork {
.collect();

let result = self.compile(&features, glyph_map);
if result.is_err() || context.emit_debug {
if result.is_err() || context.flags.contains(Flags::EMIT_DEBUG) {
if let Features::Memory(fea_content) = &*features {
write_debug_fea(context, result.is_err(), "compile failed", fea_content);
}
Expand Down
35 changes: 9 additions & 26 deletions fontbe/src/orchestration.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use fontdrasil::{
orchestration::{access_one, AccessControlList, Work, MISSING_DATA},
types::GlyphName,
};
use fontir::orchestration::{Context as FeContext, WorkId as FeWorkIdentifier};
use fontir::orchestration::{Context as FeContext, Flags, WorkId as FeWorkIdentifier};
use parking_lot::RwLock;
use write_fonts::FontBuilder;

Expand Down Expand Up @@ -68,14 +68,7 @@ pub type BeWork = dyn Work<Context, Error> + Send;
/// access with spawned tasks. Copies with access control are created to detect bad
/// execution order / mistakes, not to block actual bad actors.
pub struct Context {
// If set artifacts prior to final binary will be emitted to disk when written into Context
pub emit_ir: bool,

// If set additional debug files will be emitted to disk
pub emit_debug: bool,

// If set seek to match fontmake (python) behavior even at cost of abandoning optimizations
match_legacy: bool,
pub flags: Flags,

paths: Arc<Paths>,

Expand All @@ -92,27 +85,17 @@ pub struct Context {
impl Context {
fn copy(&self, acl: AccessControlList<AnyWorkId>) -> Context {
Context {
emit_ir: self.emit_ir,
emit_debug: self.emit_debug,
match_legacy: self.match_legacy,
flags: self.flags,
paths: self.paths.clone(),
ir: self.ir.clone(),
acl,
features: self.features.clone(),
}
}

pub fn new_root(
emit_ir: bool,
emit_debug: bool,
match_legacy: bool,
paths: Paths,
ir: &fontir::orchestration::Context,
) -> Context {
pub fn new_root(flags: Flags, paths: Paths, ir: &fontir::orchestration::Context) -> Context {
Context {
emit_ir,
emit_debug,
match_legacy,
flags,
paths: Arc::from(paths),
ir: Arc::from(ir.read_only()),
acl: AccessControlList::read_only(),
Expand All @@ -131,7 +114,7 @@ impl Context {
))
}

pub fn read_only(&self) -> Context {
pub fn copy_read_only(&self) -> Context {
self.copy(AccessControlList::read_only())
}
}
Expand All @@ -143,7 +126,7 @@ impl Context {
}

fn maybe_persist(&self, file: &Path, content: &[u8]) {
if !self.emit_ir {
if !self.flags.contains(Flags::EMIT_IR) {
return;
}
fs::write(file, content)
Expand All @@ -164,7 +147,7 @@ impl Context {

pub fn get_features(&self) -> Arc<Vec<u8>> {
let id = WorkId::Features;
self.acl.check_read_access(&id.clone().into());
self.acl.assert_read_access(&id.clone().into());
{
let rl = self.features.read();
if rl.is_some() {
Expand All @@ -179,7 +162,7 @@ impl Context {

pub fn set_features(&self, mut font: FontBuilder) {
let id = WorkId::Features;
self.acl.check_write_access(&id.clone().into());
self.acl.assert_write_access(&id.clone().into());
let font = font.build();
self.maybe_persist(&self.paths.target_file(&id), &font);
self.set_cached_features(font);
Expand Down
59 changes: 29 additions & 30 deletions fontc/src/main.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@ use fontc::{
Error,
};
use fontdrasil::{
orchestration::{access_none, access_one},
orchestration::{access_none, access_one, AccessFn},
types::GlyphName,
};
use fontir::{
glyph::create_finalize_static_metadata_work,
orchestration::{Context as FeContext, WorkId as FeWorkIdentifier},
orchestration::{Context as FeContext, Flags, WorkId as FeWorkIdentifier},
paths::Paths as IrPaths,
source::{DeleteWork, Input, Source},
stateset::StateSet,
Expand Down Expand Up @@ -66,6 +66,18 @@ struct Args {
build_dir: PathBuf,
}

impl Args {
fn flags(&self) -> Flags {
let mut flags = Flags::default();

flags.set(Flags::EMIT_IR, self.emit_ir);
flags.set(Flags::EMIT_DEBUG, self.emit_debug);
flags.set(Flags::MATCH_LEGACY, self.match_legacy);

flags
}
}

#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
struct Config {
args: Args,
Expand Down Expand Up @@ -236,7 +248,7 @@ fn add_init_static_metadata_ir_job(
},
);
} else {
workload.mark_success(FeWorkIdentifier::InitStaticMetadata.into());
workload.mark_success(FeWorkIdentifier::InitStaticMetadata);
}
Ok(())
}
Expand Down Expand Up @@ -270,7 +282,7 @@ fn add_finalize_static_metadata_ir_job(
},
);
} else {
workload.mark_success(FeWorkIdentifier::FinalizeStaticMetadata.into());
workload.mark_success(FeWorkIdentifier::FinalizeStaticMetadata);
}
Ok(())
}
Expand All @@ -294,7 +306,7 @@ fn add_feature_ir_job(
},
);
} else {
workload.mark_success(FeWorkIdentifier::Features.into());
workload.mark_success(FeWorkIdentifier::Features);
}
Ok(())
}
Expand All @@ -318,7 +330,7 @@ fn add_feature_be_job(
},
);
} else {
workload.mark_success(BeWorkIdentifier::Features.into());
workload.mark_success(BeWorkIdentifier::Features);
}
Ok(())
}
Expand Down Expand Up @@ -421,8 +433,8 @@ impl Workload {
self.job_count += 1;
}

fn mark_success(&mut self, id: AnyWorkId) {
if self.success.insert(id) {
fn mark_success(&mut self, id: impl Into<AnyWorkId>) {
if self.success.insert(id.into()) {
self.pre_success += 1;
}
}
Expand Down Expand Up @@ -562,7 +574,7 @@ struct Job {
// Things that must happen before we execute. Our task can read these things.
dependencies: HashSet<AnyWorkId>,
// Things our job needs write access to
write_access: Arc<dyn Fn(&AnyWorkId) -> bool + Send + Sync>,
write_access: AccessFn<AnyWorkId>,
}

fn create_workload(change_detector: &mut ChangeDetector) -> Result<Workload, Error> {
Expand Down Expand Up @@ -613,19 +625,11 @@ fn main() -> Result<(), Error> {
let workload = create_workload(&mut change_detector)?;

let fe_root = FeContext::new_root(
config.args.emit_ir,
config.args.emit_debug,
config.args.match_legacy,
config.args.flags(),
ir_paths,
change_detector.current_inputs.clone(),
);
let be_root = BeContext::new_root(
config.args.emit_ir,
config.args.emit_debug,
config.args.match_legacy,
be_paths,
&fe_root,
);
let be_root = BeContext::new_root(config.args.flags(), be_paths, &fe_root);
workload.exec(fe_root, be_root)?;

finish_successfully(change_detector)?;
Expand Down Expand Up @@ -770,21 +774,16 @@ mod tests {
// This will likely need to change when we start doing things like glyphs with components

let fe_root = FeContext::new_root(
config.args.emit_ir,
config.args.emit_debug,
config.args.match_legacy,
config.args.flags(),
ir_paths,
change_detector.current_inputs.clone(),
);
let be_root = BeContext::new_root(
config.args.emit_ir,
config.args.emit_debug,
config.args.match_legacy,
be_paths,
&fe_root.read_only(),
let be_root = BeContext::new_root(config.args.flags(), be_paths, &fe_root.read_only());
let mut result = TestCompile::new(
&change_detector,
fe_root.read_only(),
be_root.copy_read_only(),
);
let mut result =
TestCompile::new(&change_detector, fe_root.read_only(), be_root.read_only());

let pre_success = workload.success.clone();
while !workload.jobs_pending.is_empty() {
Expand Down
19 changes: 10 additions & 9 deletions fontdrasil/src/orchestration.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ use std::{collections::HashSet, fmt::Debug, hash::Hash, sync::Arc};

pub const MISSING_DATA: &str = "Missing data, dependency management failed us?";

/// A function that indicates whether access to something is permitted.
pub type AccessFn<I> = Arc<dyn Fn(&I) -> bool + Send + Sync>;

/// A unit of work safe to run in parallel
///
/// Naively you'd think we'd just return FnOnce + Send but that didn't want to compile
Expand All @@ -23,7 +26,7 @@ where
I: Eq + Hash + Debug,
{
// Returns true if you can write to the provided id
write_access: Arc<dyn Fn(&I) -> bool + Send + Sync>,
write_access: AccessFn<I>,

// If present, what you can access through this context
// Intent is root has None, task-specific Context only allows access to dependencies
Expand All @@ -49,18 +52,16 @@ impl<I: Eq + Hash + Debug> AccessControlList<I> {
}
}

pub fn access_one<I: Debug + Eq + Send + Sync + 'static>(
id: I,
) -> Arc<dyn Fn(&I) -> bool + Send + Sync> {
pub fn access_one<I: Eq + Send + Sync + 'static>(id: I) -> AccessFn<I> {
Arc::new(move |id2| id == *id2)
}

pub fn access_none<I: Eq + Send + Sync + 'static>() -> Arc<dyn Fn(&I) -> bool + Send + Sync> {
pub fn access_none<I: Eq + Send + Sync + 'static>() -> AccessFn<I> {
Arc::new(move |_| false)
}

impl<I: Eq + Hash + Debug> AccessControlList<I> {
pub fn check_read_access_to_any(&self, ids: &[I]) {
pub fn assert_read_access_to_any(&self, ids: &[I]) {
if self.read_mask.is_none() {
return;
}
Expand All @@ -71,7 +72,7 @@ impl<I: Eq + Hash + Debug> AccessControlList<I> {
}
}

pub fn check_read_access(&self, id: &I) {
pub fn assert_read_access(&self, id: &I) {
if !self
.read_mask
.as_ref()
Expand All @@ -82,13 +83,13 @@ impl<I: Eq + Hash + Debug> AccessControlList<I> {
}
}

pub fn check_write_access_to_any(&self, ids: &[I]) {
pub fn assert_write_access_to_any(&self, ids: &[I]) {
if !ids.iter().any(|id| (self.write_access)(id)) {
panic!("Illegal access to {ids:?}");
}
}

pub fn check_write_access(&self, id: &I) {
pub fn assert_write_access(&self, id: &I) {
if !(self.write_access)(id) {
panic!("Illegal access to {id:?}");
}
Expand Down
13 changes: 7 additions & 6 deletions fontir/src/glyph.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use crate::{
coords::NormalizedLocation,
error::WorkError,
ir::{Component, Glyph},
orchestration::{Context, IrWork},
orchestration::{Context, Flags, IrWork},
};

pub fn create_finalize_static_metadata_work() -> Box<IrWork> {
Expand All @@ -38,7 +38,7 @@ fn has_components_and_contours(glyph: &Glyph) -> bool {
.any(|inst| !inst.components.is_empty() && !inst.contours.is_empty())
}

/// Does the Glyph uses components whose transform is different at different locations designspace?
/// Does the Glyph use the same set of components, including transform, for all instances?
fn has_consistent_component_transforms(glyph: &Glyph) -> bool {
distinct_component_seqs(glyph).len() <= 1
}
Expand All @@ -55,6 +55,9 @@ fn name_for_derivative(base_name: &GlyphName, names_in_use: &IndexSet<GlyphName>
}
}

/// Returns a tuple of (simple glyph, composite glyph).
///
/// The former contains all the contours, the latter contains all the components.
fn split_glyph(glyph_order: &IndexSet<GlyphName>, original: &Glyph) -> (Glyph, Glyph) {
// Make a simple glyph by erasing the components from it
let mut simple_glyph = original.clone();
Expand Down Expand Up @@ -256,7 +259,7 @@ impl Work<Context, WorkError> for FinalizeStaticMetadataWork {
*has_consistent_component_transforms || has_components_and_contours(glyph)
})
{
if !has_consistent_component_transforms || context.match_legacy {
if !has_consistent_component_transforms || context.flags.contains(Flags::MATCH_LEGACY) {
if !has_consistent_component_transforms {
debug!(
"Coalescing'{0}' into a simple glyph because component transforms vary across the designspace",
Expand Down Expand Up @@ -367,9 +370,7 @@ mod tests {

fn test_root() -> Context {
Context::new_root(
false,
false,
true,
Default::default(),
Paths::new(Path::new("/fake/path")),
Input::new(),
)
Expand Down
Loading

0 comments on commit 15f146b

Please sign in to comment.