-
Notifications
You must be signed in to change notification settings - Fork 11
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
garretrieger
wants to merge
5
commits into
main
Choose a base branch
from
examples
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+451
−2
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
3c25a4a
Add appendix B which contains an example of the extension algorithm e…
garretrieger ff66df6
Small fix in extension example.
garretrieger 8f6ee01
Correct contained glyphs for glyph keyed patches in the example.
garretrieger 4cb02ba
Update extension example to reflect the new 'Selecting Invalidating P…
garretrieger 8868afc
Don't call preloading in example speculative.
garretrieger File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The set requires 'f' and 'P' and we load 01.gk and 04.gk, so I don't see any speculative loading for those.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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.