Skip to content

Conversation

@saecki
Copy link
Collaborator

@saecki saecki commented Nov 27, 2025

Use IndexMap to sort fonts by usage order. If the order isn't deterministic, the PDF won't be deterministic anyway.
The hashing approach previously used produces different orders on my local machine and a github actions runner.

@saecki saecki changed the title Sort fonts by usage order Use IndexMap to sort fonts by usage order Nov 27, 2025
@laurmaedje
Copy link
Collaborator

laurmaedje commented Nov 27, 2025

I wonder why the hash is not deterministic...

Also, just to double-check: There is no multi-threading going on that could affect the order, right?

@saecki
Copy link
Collaborator Author

saecki commented Nov 27, 2025

I wonder why the hash is not deterministic...

Also, just to double-check: There is no multi-threading going on that could affect the order, right?

This seems to fix the issues on 64-bit machines: https://github.com/typst/typst/actions/runs/19742335582/job/56569055041

On 32-bit machines /BaseFont and some other things seem to be different, I'm still trying to investigate that.

@saecki
Copy link
Collaborator Author

saecki commented Dec 1, 2025

So there are a couple of things here:

  • The SipHashable implementation of Prehashed also hashes the type_id, which doesn't have any stability guarantees between compilation cycles.
    • This breaks the sort order of fonts.
  • The default Hash implementation of slices writes a length prefix as a usize to any hasher.
    • This causes the differences on 32-bit platforms, I've implemented an easy fix for that here: ced12b6
      (But that's not relevant to this PR)

@laurmaedje
Copy link
Collaborator

laurmaedje commented Dec 1, 2025

This is highly relevant then: typst/typst#7503

I actually wanted to remove Prehashed from comemo for this reason. I didn't realize that krilla uses it. But LazyHash would not have this problem. Also, krilla doesn't depend on the type ID stuff I presume (unless it has prehashed trait objects), so it could also use a modified Prehashed impl without that.

But iiuc the IndexMap already fixes the problem? So maybe it's a non issue. Still the question of whether we should keep Prehashed in comemo or whether krilla is better served with its own thing.

@saecki
Copy link
Collaborator Author

saecki commented Dec 2, 2025

But iiuc the IndexMap already fixes the problem?

Yes

So maybe it's a non issue. Still the question of whether we should keep Prehashed in comemo or whether krilla is better served with its own thing.

Krilla just copy pasted the implementation. It doesn't rely on the comemo implementation.

@saecki
Copy link
Collaborator Author

saecki commented Dec 2, 2025

Investigating why the hash wasn't stable was just for peace of mind. I still think using IndexMap here is the right call.

@laurmaedje
Copy link
Collaborator

Then, this is probably good to go?

@LaurenzV
Copy link
Owner

LaurenzV commented Dec 2, 2025

Is there any way we can feasibly test this in CI? We actually already have a test with 2 fonts that is compiled repeatedly to check it says stable, but that doesn't seem to be affected by the bug.

@saecki
Copy link
Collaborator Author

saecki commented Dec 2, 2025

So the issue occurred on tests these two fonts:

I'm not sure if there is anything special about these.

@LaurenzV
Copy link
Owner

LaurenzV commented Dec 2, 2025

Hmm no idea, but if it works fine for you, then it's good from my side.

@LaurenzV LaurenzV merged commit dc28b20 into main Dec 2, 2025
12 checks passed
@LaurenzV LaurenzV deleted the font-sorting branch December 2, 2025 12:41
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.

4 participants