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

Migrate ziglyph to zg #5694

Open
mitchellh opened this issue Feb 11, 2025 · 3 comments
Open

Migrate ziglyph to zg #5694

mitchellh opened this issue Feb 11, 2025 · 3 comments
Labels
dependencies Pull requests that update a dependency file
Milestone

Comments

@mitchellh
Copy link
Contributor

Ziglyph is no longer maintained and the replacement is zg. We should migrate to zg.

I'd be willing to accept a PR to do this at any point, but this is going to become a hard requirement for our 0.14 transition when Zig 0.14 is released.

I don't know if zg has a 0.13-compatible build but it may be easiest to find that and upgrade sooner.

@pluiedev
Copy link
Member

pluiedev commented Feb 12, 2025

d667d18 seems to be the latest commit that claims to support Zig 0.13 and in terms of commit history isn't that far behind. We could try using that. One thing to note is that zg incorporates various performance improvements from Ghostty itself (mainly from https://mitchellh.com/writing/ghostty-devlog-006) and so we could probably remove a fair amount of our own code in favor of zg's implementation (which should be pretty much identical anyway).

@pluiedev pluiedev added the dependencies Pull requests that update a dependency file label Feb 12, 2025
@mitchellh
Copy link
Contributor Author

mitchellh commented Feb 12, 2025

(mainly from https://mitchellh.com/writing/ghostty-devlog-006) and so we could probably remove a fair amount of our own code in favor of zg's implementation (which should be pretty much identical anyway).

We could, but I'd rather not. Since we don't need every property, we're able to optimize our lookup tables to only include the data we need and therefore make them smaller and more cache friendly. It is trivial to build the LUTs (our unigen binary) and I think there's a benefit to having that machinery for our fast paths.

Additionally, there are optimizations we make (i.e. eliminating impossible unicode combinations) that we can do because the VT parser ensures this to be true, but generalized Unicode does not, so zg is likely slower for that reason.

These lookup tables are only used in one place though (our super hot IO path). To keep things simpler, we kept using ziglyph (and now zg) in the codepaths that aren't performance sensitive. And we should keep doing that.

@pluiedev pluiedev mentioned this issue Feb 13, 2025
14 tasks
@pluiedev pluiedev added this to the 1.2.0 milestone Feb 13, 2025
@pluiedev
Copy link
Member

pluiedev commented Mar 7, 2025

So I was looking at zg's API design and one thing I immediately noticed was that it's not nearly as accessible as ziglyph's is. We need to pass around a central "context" struct that holds all of the information zg uses since now it has to decompress tables at runtime...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

No branches or pull requests

2 participants