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

m(breaking): Simplify internal storage and lookup mechanisms #45

Closed
wants to merge 1 commit into from

Conversation

notgull
Copy link
Contributor

@notgull notgull commented Feb 18, 2023

This PR replaces the current Vec and next_id-based storage strategy with the Slab type. Slab is similar to the current storage strategy, except that the key corresponds directly to the index in the underlying Vec. This means that the lookup process is an O(1) process as opposed to an O(n) process.

The goal is to replace this bit of self-referencing code in cosmic-text so that one can gain access to the underlying &mut Database.

The only wrinkle that that Slab<T> doesn't directly correspond to an &[T], so the faces() function has to be an impl Iterator<&FaceInfo> instead of an &[FaceInfo]. This is a breaking change.

@RazrFalcon
Copy link
Owner

I don't think this is a good idea. First, I don't want any dependencies. And second, to my understanding, slab reuses indexes, which can lead to pretty bad results.

If you want O(1) indexing, then we need either a HashMap (requires std) or simply do not remove faces. But the later one would mean that we would need to have a removed/disabled flag or something.

Or, since the current ID is ordered, we can use binary search, which should be faster then the current approach.
I guess the current logic is a leftover from times when we had u64 UUID instead of just u32 indices.

@notgull
Copy link
Contributor Author

notgull commented Feb 18, 2023

I don't think this is a good idea. First, I don't want any dependencies.

slab with no-default-features consists of a single file, so it's a very lightweight dependency. On top of that, slab is a dependency of almost every async framework and soon to be a dependency of calloop (Smithay/calloop#123), so in the cases where fontdb is used it probably uses slab already anyways.

And second, to my understanding, slab reuses indexes, which can lead to pretty bad results.

This only ends up being an issue when fonts are removed, which is a rarely used operation. On top of that, if a font is removed, that implies that the key isn't being used anymore by whatever's using fontdb at the higher level. If this is still a concern, slotmap doesn't have this problem.

If you want O(1) indexing, then we need either a HashMap (requires std)

This would be possible without libstd, but we would need to import hashbrown alongside a good-enough hashing algorithm (ahash?). Not only does this require significantly more dependencies, it would raise this crate's MSRV to 1.61 at a minimum.

or simply do not remove faces. But the later one would mean that we would need to have a removed/disabled flag or something.

This is possible, but would leak memory.

Or, since the current ID is ordered, we can use binary search, which should be faster then the current approach.

Binary search is O(n log n), and the goal is for O(1) search so that caching the search isn't necessary.

@RazrFalcon
Copy link
Owner

This is possible, but would leak memory.

This only ends up being an issue when fonts are removed, which is a rarely used operation

I would say changing remove to disable and using ID for indexing is a far easier solution.

@notgull
Copy link
Contributor Author

notgull commented Feb 18, 2023

Okay, I've opened #46 to supersede this PR.

@notgull notgull closed this Feb 18, 2023
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