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

Rework the copy indices mechanism #252

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Rework the copy indices mechanism #252

wants to merge 6 commits into from

Conversation

garretrieger
Copy link
Contributor

@garretrieger garretrieger commented Feb 5, 2025

Instead of producing multiple subset definitions, define the intersection check in terms of whether referenced child entries would intersect. This largely provides the same functionality while overall making the mechanism simpler to reason about. Additionally it provides a path to a more efficient client side implementation, which can now cache the intersection checks on prior entries to quickly compute down stream entry intersection results. The prior approach requires in some cases fully expanding out the subset definition unions for each entry to correctly compute intersection results.

Also adds an appendix B example using child indices to demonstrate how UVS specific patches can be loaded. Fixes #245.


Preview | Diff

Instead of producing multiple subset definitions, define the intersection check in terms of whether the referenced entries would intersect. This largely provides the same functionality while overall making the mechanism simpler to reason about. Additionally it provides a path to a more efficient client side implementation, which could cache the intersection checks on prior entries to quickly compute future entry intersection results.
@garretrieger garretrieger requested a review from skef February 5, 2025 19:14
garretrieger added a commit to googlefonts/fontations that referenced this pull request Feb 5, 2025
Copy indices becomes child indices. Intersection is now based on the intersection of the child entries instead of forming a union of the child entry subset definitions.
garretrieger added a commit to googlefonts/fontations that referenced this pull request Feb 5, 2025
Copy indices becomes child indices. Intersection is now based on the intersection of the child entries instead of forming a union of the child entry subset definitions.
Copy link
Contributor

@svgeesus svgeesus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems good (I read the preview, because the diff was hard for me to follow in places) and seems to be clearer than the previous method.

Overview.bs Outdated Show resolved Hide resolved
@@ -540,8 +543,13 @@ The algorithm:
When checking design space sets for intersection, they intersect if there is at least one pair of intersecting segments
(tags are equal and the ranges intersect).

2. If all sets checked in step 1 intersect, then return true for <var>intersects</var> otherwise false.
2. If one or more sets checked in step 1 did not intersect, then return false for <var>intersects</var>.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are 2 and 4 described correctly here? I haven't looked into the details before this review but naively I would have expected that in disjuctive mode this should return true if either the base set or one of the child entries intersects. You have the base set acting as a sort of "screen" so I'm wondering if that's intentional and if it is why you want it that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I intended the match mode setting to apply only to the child entries, but I can see an argument for it applying to the entry as a whole (and that would actually match the previous behaviour). I don't have a specific use case in mind for the way I've currently specified it so I'm good with changing to apply to the entry as a whole.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched to have the whole entry respect the match mode and added an example illustrating a disjunctive match with the entry's subset def and a child entry.

Copy link
Contributor Author

@garretrieger garretrieger Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually ran into an issue with this so going to switch back to the initial approach (subset def is a filter). The problem is that it's pretty typical to have an entry that has an empty subset definition and then list its conditions using only child entries. In the disjunctive case if we treat the whole entry as disjunctive then the entry's empty subset definition always matches everything (since empty sets are considered wild cards for matching) and since it's treated disjunctively that means the entry will always match everything as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this maybe a time to compromise on matching formal logic? The current convention works well for empty sets but I worry it works poorly for others -- we've basically made it so that there's no easy way for an entry to "add onto" what's included via the child entries (unless I'm misunderstanding).

Copy link
Contributor Author

@garretrieger garretrieger Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If needed you can achieve a fully disjunctive entry by have an empty subset definition (which matches everyting) and then specifying the remaining conditions using child entries.

So for example

entry {
  code points: {a, b, c}
  child entry {
    code points: {d, e, f}
  }
  disjunctive = true (where this was intended to apply to the entries codepoints as well)
}

Can be represented as:

entry {
  codepoints: {}
  child entry {
    code points: {a, b, c}
  }
  child entry {
    code points: {d, e, f}
  }
  disjunctive = true
}

Overview.bs Outdated Show resolved Hide resolved

4. Invoke [$Check entry intersection$] for each child entry referenced in <var>mapping entry</var> with <var>subset definition</var>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this is The Algorithm it might be good to specify some sort of protection against cycles here. That's actually a little tricky because you can't just keep a hash of entries you've looked at -- more than child might reference the same one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cycles are prevented by requiring child entry indices to always point to previous entries. Specifically this requirement: "If a childEntryIndices is greater than or equal to the length of prior entry list then, this encoding is invalid return an error.". This requirement ensures the graph formed by them is a DAG.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK - in that case do we have potential conflicts between the patch map ordering for inclusion purposes and the ordering for tie-breaking? The tie-breaking aspect is sort of a corner case so it's not obvious there would be a problem but I'm wondering if things could get tricky and/or annoying in some circumstances.

Copy link
Contributor Author

@garretrieger garretrieger Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this will typically be an issue since table keyed patch entries aren't expected to make use of child entries. However if it does come up it can be resolved by using child entries, basically you can set up all of the conditions as ignored entries at the start of the encoding and then have the non-ignored entries in whatever order you wish (after the ignored ones) where each one references a single ignored entry from earlier which defines the desired condition.

Overview.bs Outdated Show resolved Hide resolved
Overview.bs Outdated Show resolved Hide resolved
Overview.bs Outdated
@@ -734,17 +770,18 @@ round trips.
The following selection criteria minimizes round trips and must be used by the client when selecting a single partial or full invalidation patch in step 8
of [$Extend an Incremental Font Subset$]:

1. For each candidate entry compute the set intersection between each subset definition in the entry and the target subset definition. Union
the resulting intersections together into a single subset definition.
1. For each candidate entry compute a total subset definition by unioning that entry's subset definition and the subset definition of all child entries
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused about this. Why are we unioning if and when the child entry match mode is conjunctive?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this procedure we are trying to assess what code points, features, and design space an entry has data for. As an example whether a patch is loaded when (A or B), or when (A and B) both cases indicate that it has data related to both A and B.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I understand that point broadly but now I'm worried about the overall selection system. If I'm tracking this right in my head, this is where we pick the next patch to apply, and the way we've specified things we can have competing paths to incorporating the needed codepoints. So, for example, patches can map overlapping sets of codepoints such as a patch for A and a patch for B but also a patch for A+B.

If this is in the same space we're doing that, and we union conjunctive patches, it seems like it can make patches that do something small and specific "look larger" than patches that do something big and general.

Or am I just confusing myself?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I just realized that the answer is something like "we only need to use this mechanism for the glyph-keyed patches, and ordering/tie-breaking is only relevant to the table-keyed patches". So this would just be a new mine in the field of unexplored generality we are creating.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I'm actually just realizing that we may want to use conjunctive conditions for table keyed patches, and I don't believe any changes are needed in this procedure for those to work correctly:

The primary goal of this process is to minimize round trips by avoiding the selection of patches that are fully encompassed by some other larger patch which will be needed (which is what the super set stuff aims to solve). One nice thing about child entries mechanism I'm just realizing is it could be used to better specify the desired selection order for table keyed patches. For example in a case where you have table keyed patches for A, B, A+B. We've typically thought the mappings would be A -> patch for A, B -> patch for B, and A union B -> patch for A+B. Which then necessitates an appropriate entry ordering so that if a client is selecting for only A then entry for just A will have priority over A+B.

Instead if the entry for patch A+B used the condition (A and B), then the A+B patch would only be considered for loading when code points from both A and B are present. From there for this procedure will consider the union of A and B which will cause the A+B patch to be chosen over either individual A and B patches. Note how we didn't need to depend on a specific entry ordering at any point to get the desirable outcome. This also demonstrates how using A union B in this procedure for an (A and B) gets the correct outcome.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not ruling out that there may be convoluted ways to specify a mapping that could producing a suboptimal selection, but I don't see how any reasonably constructed encoding would end up in such a situation.

This is presumably a tautology, on your approach, because to be a "reasonably constructed" encoding would entail not being suboptimal. "They should just have been smarter."

This whole specification, at this point, is very convoluted. Desirable and expected encodings are convoluted. "Convoluted" is not a relevant epithet.

I will think through some cases but maybe you could also think through some alternatives? Like, why can't conjunctive patches have an extra field indicating, in some way comparable to codepoint count, its "size"? What would that break?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're engineering a system that we expect can be used in multiple ways. We're ensuring that that system can be used in the ways we need to use it, and don't want to close off other options. I presume that one of the reasons we don't want to close off other options, among others, is that we may find that we need to take advantage of some of those other options after all.

We have a system for ordering patches among competing options. The goal of that system is to load patches that make more sense first, where "makes more sense" is pretty close to when the patch "does more". We're technically not adding a case with these changes where the order inverts, but the case that was there only applies to independent patches, where ordering is basically irrelevant. Now it will apply to table-keyed patches, where ordering can matter.

You're saying: "show me a case where this will screw things up in practice". I don't know, today, everything that we may need to do in practice. I don't think you do either. One say "we'll cross that bridge when we come to it" but I simply don't believe that's how good engineering works. What makes it more likely a system will be able to do beyond what one immediately envisions is if the features make sense together. That generally speaking each has its own area of functionality and responsibility and that one can understand how that's true.

I don't want to discover, in six months, that we have to rewrite substantial parts of the spec in order to allow some new small thing because we added a bunch of stuff that doesn't make conceptual sense on the argument "it didn't break anything at the time".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not ruling out that there may be convoluted ways to specify a mapping that could producing a suboptimal selection, but I don't see how any reasonably constructed encoding would end up in such a situation.

This is presumably a tautology, on your approach, because to be a "reasonably constructed" encoding would entail not being suboptimal. "They should just have been smarter."

The invalidating selection algorithm was designed around a couple of assumptions and will produce optimal results so long as those assumptions hold. So given that here's a take at a more formal definition of when the selection criteria will produce optimal results and when it may run into trouble:

The "coverage" of an entry is defined as the union of all subset definitions of that entry and all reachable child entries. The invalidation selection algorithm was designed around these three assumptions:

  1. When patches are applied they update the fonts mapping entries to reflect the effects of their application by removing coverage from entries (and their patches) for data that has been added by the application (ie. if something adds all data needed for A, an entry for A U B would be updated to just B).
  2. Each entry's coverage is "accurate", which more specifically means:
    • An entry adds data related only to it's declared coverage, (ie. if coverage is {a, b, c} and data for d is included that fails this).
    • The coverage is as minimal as possible without violating the previous point. (ie. if coverage is {a, b, c} but data for only b and c are added this is not minimal).
  3. The physical order of entries in the mapping is from smallest to largest in terms of size in bytes of the associated patch.

Note: when I say "data related to" that could also be conditional data (eg. {f} AND {i} -> patch with only fi ligature data) would satisfy these conditions. The important thing here is that when applied the ligature patch would respect assumption 1 and not remove other entries that contained the f and i glyphs.

If all of the entries for invalidating patches in an encoding satisfy these three conditions, then the selection criteria will produce a selection that minimize round trips. Otherwise if an encoding does not satisfy these three conditions then there is no gaurantee the procedure will produce an optimal result, that is may take more round trips than strictly necessary. If an encoder implementation is choosing to violate one or more of these assumption they will need to be careful and work out how whatever they're doing will behave under the selection procedure.

These three conditions are straightforward for an encoder implementation to meet and align with what we consider best practices for the table keyed encodings. Additionally they're not overly restrictive and leave room for flexibility in the encoding specifics. For example no restrictions are placed on overlaps between entries. Partial, full, no overlaps, or a mix of all of those will work correctly. Likewise these restrictions don't prevent the use of conjunctive or other composite matching conditions. A nice side effect of writing the procedure in terms of coverage is that it makes reasoning about how complex conditions will be treated very straightforward.

I think it would be a good idea to add something along these lines into the encoder guidance as it provides a clear guideline for how to produce encodings that will work optimally with the selection criteria.

I will think through some cases but maybe you could also think through some alternatives? Like, why can't conjunctive patches have an extra field indicating, in some way comparable to codepoint count, its "size"? What would that break?

The entry ordering currently provides size information. I don't see a need for an explicitly recorded size, since the selection procedure only cares about the relative size of things. Furthermore size is actually a secondary metric for determining selections used only for tie breaking. The primary and most important metric is roughly what we previously called "stuff I need" and this depends on the interaction of both the target subset definition and what's actually in the patch. We use an intersection between the target subset definition and the entry's coverage as a proxy for this. If we wanted more information available for the selection procedure that would likely need to come in the form of per codepoint information (size or otherwise), which I don't believe would materially improve selection quality and would significantly bloat the encoded mapping sizes. There's a fundamental tradeoff here where if we want to improve selection quality we need more complete information, but that information has a cost to encode. The current selection algorithm strikes a very good balance. It produces optimal results for all cases we expect to need and it operates with a minimal amount of information (no additional information beyond the coverage data which is already present).

To justify increasing encoding sizes I'd want to see concrete use cases that meaningfully improve performance to justify the increase. Basically are there table keyed encoding schemes we'd want to produce that will produce better performance then the existing "one round trip" scheme, but won't work correctly with the current selection algorithm?

I don't want to discover, in six months, that we have to rewrite substantial parts of the spec in order to allow some new small thing because we added a bunch of stuff that doesn't make conceptual sense on the argument "it didn't break anything at the time".

At this point we know, given the current capabilities, that we can produce a performant IFT font. We have a real working proof of concept showing that. When we did analysis on the potential performance of IFT a couple of years back we had this idea of an "ideal implementation" which was a theoretical one that transferred exactly as much data as needed in any given extension. This ideal implementation is the theoretical lower bound for performance of an IFT font. I would assert that with current state of the art encoding schemes we've developed we are starting to approach the practical lower bound (which is higher then the theoretical one since overhead is unavoidable) and all that's left are smaller incremental improvements. As an aside, once I wrap up some remaining encoder implementation work I'd like to do some analysis and see where we are with respect to the theoretical lower bounds for a variety of archetypal fonts.

The questions around the selection procedure to me are more about whether we're leaving enough flexibility in the future for these incremental improvements. The selection procedure currently rests on a few assumptions as discussed above, which in turn adds some amount of restriction on encodings. So the question is: are those restrictions reasonable, do the give enough flexibility? If we don't think they're reasonable, then lifting those restrictions can be done but will most likely impose some additional cost in the encoding size. In my opinion the restrictions are reasonable, and leave sufficient room for additional exploration and advancement in encoding schemes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll read through this more thoroughly, but how do we expect criterion 3 to be met for conjunctive patches in the general case when child entries must only point to previous entries? A conjunctive A, B, C patch will typically be smaller than any of those individual patches.

From what I understand, the answer here is "sure, the algorithm was designed around these assumptions but it turns out it does't matter for these patches so we can just set aside the assumptions."

You point out that ordering is only used for tie-breaking, which is true in the current model, but that's the problem given that our proxy for "size" isn't accurate anymore. The union criterion works for disjunctive patches, it doesn't for conjunctive patches.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll read through this more thoroughly, but how do we expect criterion 3 to be met for conjunctive patches in the general case when child entries must only point to previous entries?

I mentioned this in one of the other comment chains but you can use ignored entries to achieve arbitrary ordering of the non-ignored entries even when using child entries. In this case:

entry[0]: A, ignored
entry[1]: B, ignored
entry[2]: C, ignored
entry[3]: children = {0, 1, 2}, conjunctive
entry[4]: children = {0}
...

A conjunctive A, B, C patch will typically be smaller than any of those individual patches.

In the table keyed case this isn't typically true. A conjunctive matching condition says load this thing if A, B, and C are simultaneously present. In expected usage that's because the patch contains data for all of A, B, and C and shouldn't be loaded unless all three are needed (ie. if we only have A, B don't use this patch, find another one). Condition 2 requires that the patch which includes A, B, C in it's conditions should contain data for all three. Of course there's a corner case which I mentioned above where you could have split of some same ligature data into a separate patch, but that will still resolve correctly under the procedure since the ligature data patch won't exclude the other relevant patches from selection (I don't see a reason why you'd actually do this, but it will still work).

From what I understand, the answer here is "sure, the algorithm was designed around these assumptions but it turns out it does't matter for these patches so we can just set aside the assumptions."

You point out that ordering is only used for tie-breaking, which is true in the current model, but that's the problem given that our proxy for "size" isn't accurate anymore. The union criterion works for disjunctive patches, it doesn't for conjunctive patches.

If an encoding meets the 3 conditions then the union criterion works correctly even for conjunctive matching conditions. Whether a match is conjunctive or disjunctive doesn't communicate any meaningful information for the purpose of invalidating patch selection (it's of course meaningful when we are selecting the initial candidates via intersection checks). If a condition is (A or B) absent additional information we have no reason to believe that patch will be smaller or larger than one with condition (A and B).

There are valid use cases to use either of (A or B), (A and B) with a patch that adds all data for A + B. Like wise there's cases where either of (A or B), (A and B) could be used for a more conditional type patch that contains a small amount of supplemental data (eg. ligature glyphs for AND, shared glyphs for OR). Unless we have additional data about the contents of the patch, which means more data encoded in the mapping, then we can't do anything other than conclude (for the purposes of invalidating patch selection) that the conditions are roughly equal in cost.

garretrieger added a commit to googlefonts/fontations that referenced this pull request Feb 6, 2025
Copy indices becomes child indices. Intersection is now based on the intersection of the child entries instead of forming a union of the child entry subset definitions.
garretrieger added a commit to w3c/ift-encoder that referenced this pull request Feb 6, 2025
@skef
Copy link
Contributor

skef commented Feb 7, 2025

Another thing I'm wondering:

These changes add a child patch inclusion mechanism but eliminate the ability to specify multiple definitions for an entry. That's going to work for most cases but it means that if you don't already have a child entry with the right contents you're out of luck.

I think for the sake of simplicity of the algorithm that's still the right choice. But it might be handy to be able to have "pure child" entries -- so entries that can be included as children but will otherwise be ignored as their own entries. Seems like that addresses the just mentioned problem and also (sort of ) opens up the possibility of support for a larger domain of boolean expressions, should those be useful somehow.

(There's no "not", of course, so that domain would not include arbitrary boolean expressions. Hmm -- should there be? You wouldn't typically want a "bare" or I guess "top" not because it would draw in codepoints that aren't supported by the font. But I can imagine cases where a not would be useful in a sub-expression.)

@garretrieger
Copy link
Contributor Author

garretrieger commented Feb 7, 2025

I think for the sake of simplicity of the algorithm that's still the right choice. But it might be handy to be able to have "pure child" entries -- so entries that can be included as children but will otherwise be ignored as their own entries. Seems like that addresses the just mentioned problem and also (sort of ) opens up the possibility of support for a larger domain of boolean expressions, should those be useful somehow.

We actually have this capability currently. Format 2 entries have an ignore bit (See step 11 of https://w3c.github.io/IFT/Overview.html#interpreting-patch-map-format-2). When an ignore bit is set on an entry it will be excluded from the list of entries which patch selection draws from but can still be utilized as a child entry (where it will match as expected), and yeah as you suggested this opens up the possibility of constructing complex multi level boolean conditions if desired.

(There's no "not", of course, so that domain would not include arbitrary boolean expressions. Hmm -- should there be? You wouldn't typically want a "bare" or I guess "top" not because it would draw in codepoints that aren't supported by the font. But I can imagine cases where a not would be useful in a sub-expression.)

Interesting, I don't immediately have a case for this in mind but certainly something to consider.

@skef
Copy link
Contributor

skef commented Feb 13, 2025

Just treat @skef as off the reviewer list for this PR. You have another approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add appendix B example that shows usage of a multiple subset definition patch mapping
3 participants