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

Add appendix B which contains an example of the extension algorithm execution. #237

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

Conversation

garretrieger
Copy link
Contributor

@garretrieger garretrieger commented Nov 14, 2024

The example shows howed a mixed (table + glyph keyed) IFT font would typically be processed by a client.

For #216


Preview | Diff

Overview.bs Outdated Show resolved Hide resolved
Overview.bs Outdated Show resolved Hide resolved

<b>Optimized Extension Example</b>

Note: this example execution is described as it would be implemented in an optimized client which aims to reduce the number of round trips needing to be
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 one aspect of this description needs updating in light of #238 (as expected) and the second isn't accurate relative to the example. This is will now be following the selection criteria rather than "making choices". And I don't think it's doing any speculative loading -- it's choosing 3 as it should. Or maybe that's what was meant.

I think going forward we can talk about the algorithm now that it's nailed down, and occasionally talk about speculative loading. For the latter I want to get away from the term "preload" because we're using that for adding patches to a file on the server side.

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've updated the later step that talks about invalidating patch selection to reflect the changes in #238. Also as suggested changed preload to speculative.

For this particular note the speculative loading I'm talking about is for the glyph keyed patches (so not related to invalidating patch selection), I added some text to clarify that.

Copy link
Contributor

Choose a reason for hiding this comment

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

For this particular note the speculative loading I'm talking about is for the glyph keyed patches (so not related to invalidating patch selection), I added some text to clarify that.

The set requires 'f' and 'P' and we load 01.gk and 04.gk, so I don't see any speculative loading for those.

Copy link
Contributor Author

@garretrieger garretrieger Nov 18, 2024

Choose a reason for hiding this comment

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

These are speculative in the sense that we have to get and apply the table keyed patch first and we don't know for sure exactly what it will do. Although unlikely it could modify the entry list for the glyph keyed patches and remove 01.gk and 04.gk entries making the loads for those two unnecessary. Maybe speculative isn't quite the right word here, how about preemptive?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, hmm. From the text further down:

The table keyed patch //foo.bar/04.tk is partially invalidating which means the two glyph keyed patches will be remain valid after the application of //foo.bar/04.tk. Thus the client can expect that the two glyph keyed patches will be required in future iterations and preemptively begin the loads for those two patches.

What's ambiguous about the situation such that either "speculative" or "preemptive" really applies? If 04.tk could make the other two patches redundant, it would be one thing, but it can't.

I suppose loading patches in parallel can be seen as an "optimization" but in practice it will kill performance not to, so I don't think we should present it as "optional" except in the loosest sense.

Why is there any linkage between the two patch groups in the algorithm in the first place? If 01.gk arrives before 04.tk, can't you (in terms of correctness) apply it first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the invalidation modes guarantees that the glyph keyed patches in this case are always safe to apply. However, the invalidation rules don't prevent a table keyed patch from rewriting the glyph keyed map to point to a different set of URIs that still use the same compatibility base (eg. how we handle variable axis extension in the prototype). So there's no harm in applying the glyph keyed patches early, but it's possible (though unlikely) they may become unnecessary if they are superseded by new patches. Definitely agree that saving the round trip is always worth it even in the odd case where you grab patches you don't end up needing. I'll reword this a bit to tone down the "speculative" nature of this behaviour and just present it as what a good implementation should do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, updated the text. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

However, the invalidation rules don't prevent a table keyed patch from rewriting the glyph keyed map to point to a different set of URIs that still use the same compatibility base (eg. how we handle variable axis extension in the prototype).

I thought this was the purpose for partial vs full invalidation patches. Full invalidation patches invalidate the patches in both IFT tables (or something approximating that) while partial invalidation patches only invalidate those in their own table. The example explicitly says the patches are from two separate tables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Invalidation is currently defined in terms of the compatibility IDs, that is a patch which invalidates a mapping will change the compatibility ID on that mapping which means any existing patches can no longer be applied. So a partial invalidation patch only promises not to change the compatibility ID of the other mapping table, but nothing in the text prohibits a partial invalidation patch from modifying the contents of the mapping (eg. swapping in a new set of patch uris so long as they all use the same compat IDs).

That said, I'm not sure this is a use case we actually need to support, so maybe we should update the text of the invalidation section to also say the patch also won't modify the contents of the mapping.

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.

2 participants