Implement subgraph splitting for overflow resolution

[cmyr]

A port of the harfbuzz implementation; when overflows occur in subgraphs with multiple 32-bit roots (e.g. extension subtables sharing coverage tables or similar) we will iteratively select some subset of those roots and move them to another subgraph, duplicating and dependencies.

based on #386, which should go in first.

[rsheeter]

I suggest we might err on the side of issues over comments for this sort of thing, with possible exception for cases where we're going to fix it in O(days)

[cmyr]

Oops this comment is out of date; we check the return and convert to an error in the dump_table fn.

[rsheeter]

Can you write this as while !overflows.is_empty() instead of sticking a return in the middle? Hm. Or structure it as iteration?

[cmyr]

The problem is that we aren't always going to continue until there are no overflows left; sometimes overflow is unavoidable.

The control flow here is:

• check if there are overflows
• if there are none, return
• if there are some, try to make progress on resolving them
• if we don't make progress, break the loop and print diagnostics

This should always terminate, since we exit the loop if either there are no overflows, or if there are overflows but there are no splittable subgraphs remaining.

[rsheeter]

The comment above reads as if we are going to continue until there are none so that might need a fix then. IMHO while primary-exit-condition is still nicer, even if there is another path toward failure.

What about using Result<(), SomeErrType> so you can use normal control flow, e.g. try_splitting_subgraphs(...)??

[rsheeter]

if debug is not enabled early exit

[rsheeter]

also should be behind log feature per other conversation? - np if that's out of scope here though.

[cmyr]

yea we don't have a log feature yet, will revisit that when I make that change (opened #397 to track this)

[rsheeter]

It looks like the early exit for log disabled - avoiding the iterations and zips to produce logs we never write - was not done?

[cmyr]

We don't have a log feature, so there's nothing to use to gate this code. In any case this exists right now as a utility for me to use while I am implementing the splitting/promotion stuff.

[rsheeter]

Optional, looks like this would fit very naturally as .iter().filter()?

[cmyr]

It doesn't feel like a clear improvement, because i want to reuse the child_space binding outside of the filter. 🤷

for overflow in overflows.iter().filter(|overflow| {
            let child_space = self.nodes[&overflow.child].space;
            // we only isolate subgraphs in wide-space
            !(!child_space.is_custom() || self.num_roots_per_space[&child_space] <= 1)
        }) {
            let child_space = self.nodes[&overflow.child].space;
            let root = self.find_root_of_space(overflow.child);
            debug_assert_eq!(self.nodes[&root].space, child_space);
            to_isolate
                .entry(child_space)
                .or_insert_with(HashSet::new)
                .insert(root);
        }

[rsheeter]

At a glance it looks like you could write this as to_isolate = overflows.iter.filter.map.collect?

[cmyr]

The reason I do this in a loop is because I want to use the 'entry' method on HashMap, which doesn't work with collect. Basically I'm finding multiple roots per space.

[rsheeter]

Diagram appreciated, without it the test would be inscrutable

[cmyr]

Agreed, I found it impossible to come up with good test cases without drawing them out first.

[dfrg]

QQ: ObjectIds are allocated with a global usize counter. Should we change this to an explicit u64 (or allocate ids locally in the graph) to avoid potential overflows if someone does decide to run this on a 32-bit system?

[cmyr]

QQ: ObjectIds are allocated with a global usize counter. Should we change this to an explicit u64 (or allocate ids locally in the graph) to avoid potential overflows if someone does decide to run this on a 32-bit system?

Hmm... I would expect the process to only be running for a specific compilation, which even in the case of a very large multi-access font it's hard to imagine there being 4.2e9 unique objects? And in that case I would definitely expect us to be on a 64-bit system. That said, it wouldn't hurt...