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

Unify Attrs and AttrsOwned #100

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

geieredgar
Copy link
Contributor

@geieredgar geieredgar commented Mar 6, 2023

Currently, there exist the borrowing Attrs and the owning AttrsOwned, which only differ in their usage of Family vs FamilyOwned. In theory, this allows passing Attrs without having to allocate. In practice, we have the following situation:

  1. FontSystem::get_font_matches wants to prevent an allocation in the most-common case that a cached match already exists, but it can't (see
    .entry(AttrsOwned::new(attrs))
    ), because we cannot implement Borrow<Attrs> for AttrsOwned.
  2. We need to store AttrsOwned in AttrsList, so it is almost impossible to not have an Attrs turn into an AttrsOwned somewhere down the line.

This PR removes the old Attrs, renames AttrsOwned to Attrs and adds an AttrsBuilder, which has the same helper methods as the old Attrs. This PR also includes several changes to prevent allocations in many cases:

  • FamilyOwned uses a Cow<'static, str> instead of String, allowing the use of string literals without allocating.
  • AttrsBuilder::family takes an impl Into<FamilyOwned>, which is only implemented for FamilyOwned or Family<'static>, so users need to explicity use FamilyOwned::new if they want to use a non static Family.
  • Attrs implements AsRef<Attrs> and From<&Attrs>, allowing methods that might need to store the Attrs to accept both values and references of Attrs, while at the same time preventing an extra allocation when a value is passed.
  • As a side effect of Attrs now not being Copy, users have to explicitly use clone, which makes potential allocations more obvious.

This PR also changes FontSystem::get_font_matches to take an impl AsRef<Attrs> + Into<Attrs> parameter, which it only clones if (a) the passed argument is a reference and (b) the cache does not contain an equivalent Attrs. So this change leads to probably less allocations overall.

@jackpot51
Copy link
Member

This now has conflicts after merging #97

@geieredgar geieredgar force-pushed the attrs branch 2 times, most recently from 31da20e to 829258a Compare March 18, 2023 08:48
@geieredgar
Copy link
Contributor Author

The conflicts are resolved, but the CI fails right now, caused by duplicate syn dependency, unrelated to my changes.

@Imberflur
Copy link
Contributor

FWIW point 1 can probably be addressed via using hashbrown with HashMap::entry_ref + Equivalent.

Copy link
Contributor

@genusistimelord genusistimelord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the changes here seem reasonable as it will help simplify the overall usage of Attr's.

My only Concern is how will this affect the overall performance since we are moving towards clones more than copy. @jackpot51 what do you think?

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