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.

Sign up for GitHub

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

Jump to bottom

Graph isolation fix #591

Merged
merged 2 commits into from
Sep 7, 2023
Merged

Graph isolation fix #591

merged 2 commits into from
Sep 7, 2023

Conversation

cmyr
Copy link
Member

@cmyr cmyr commented Sep 6, 2023

This is a roll up of two commits:

  • the first is a set of changes to ensure that graph packing is deterministic; in practice this means replacing HashSet and HashMap with BTreeSet and BTreeMap in places where we later iterate over the collection.
  • the second fixes a subtle bug in our logic when we are attempting to isolate some subgraph: we were not tracking whether we had visited a node previously when counting edges, which meant that in certain circumstances, when a node pointed to the same child multiple times, we would count subsequent links in that subgraph multiple times, which could cause us to fail to correctly duplicate nodes when needed. This bug was a large part of what I was banging my head against last week.

cc @garretrieger; the relevant stuff here is in 1bc5900.

@cmyr
[write-fonts] Deterministic graph packing
12af289 
This replaces HashMap & HashSet with BTreeMap and BTreeSet in various
places where we iterate over the collection. This ensures that our
behaviour is never dependent on insertion order.
@garretrieger garretrieger mentioned this pull request Sep 6, 2023
Closed
@cmyr
[write-fonts] Fix double-counting some graph nodes
261b3f3 
This was a subtle bug in our packing code, where under certain
conditions we would double count inbound edges to certain nodes, causing
us not to duplicate them when we should have.
@cmyr
Copy link
Member Author

cmyr commented Sep 7, 2023

updated this to copy the harfbuzz impl, which reuses the subgraph map to check for 'visited' state, saving us from needing a second data structure.

@cmyr cmyr merged commit b81bd09 into main Sep 7, 2023
9 checks passed
@cmyr cmyr deleted the graph-isolation-fix branch September 7, 2023 16:01
@cmyr cmyr mentioned this pull request Dec 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rsheeter rsheeter rsheeter approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cmyr @rsheeter