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

Remove all instances of HasFlag #426

Merged
merged 1 commit into from
Dec 16, 2024
Merged

Conversation

JimBobSquarePants
Copy link
Member

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

See #425 (reply in thread)

HasFlags sometimes still boxes (in tier 0 compilation) if JIT does not optimize it away.

@nolife99 I profiled a couple of unit tests and saw a reduction however it would be useful if you could give this a try by referencing the code directly and report back.

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 7 changed files in this pull request and generated no suggestions.

Files not reviewed (2)
  • src/SixLabors.Fonts/Font.cs: Evaluated as low risk
  • src/SixLabors.Fonts/FontDescription.cs: Evaluated as low risk
@nolife99
Copy link

nolife99 commented Dec 5, 2024

I did implement this myself yesterday (exactly the same as you did), and it showed a pretty good improvement in memory usage and GC performance

@JimBobSquarePants
Copy link
Member Author

@nolife99 You wouldn't have any screenshots you can share or some sample code I can use to reproduce would you?

@nolife99
Copy link

nolife99 commented Dec 6, 2024

    public Image<Rgba32> CreateBitmap(string text,
        string fontName,
        float fontSize,
        Vector2 padding,
        out Vector2 textureSize)
    {
        if (string.IsNullOrEmpty(text)) text = " ";

        const float dpi = 72f; 

        // replace below with a Font instance, '96 * fontSize / dpi' is the font size
        var font = getFont(fontName, 96 * fontSize / dpi, FontStyle.Regular);

        var foundMetrics = font.Family.TryGetMetrics(FontStyle.Regular, out var metrics);
        RichTextOptions options = new(font)
        {
            HorizontalAlignment = HorizontalAlignment.Center,
            VerticalAlignment = VerticalAlignment.Center,
            LineSpacing = foundMetrics ? Math.Max(metrics.VerticalMetrics.LineHeight * .001f, 1) : 1,
            Dpi = dpi
        };

        var measuredSize = TextMeasurer.MeasureAdvance(text, options);
        var width = (int)(measuredSize.Width + padding.X * 2 + 1);
        var height = (int)(measuredSize.Height + padding.Y * 2 + 1);

        textureSize = new(width, height);

        Image<Rgba32> bitmap = new(width, height);
        bitmap.Mutate(b =>
        {
            RichTextOptions drawOptions = new(font) { Origin = padding };
            b.DrawText(new(drawOptions) { Origin = padding + Vector2.One }, text, Color.White).DrawText(drawOptions, text, Color.FromRgba(0, 0, 0, 220));
        });

        return bitmap;
    }

    Image<Rgba32> generateGlyph(char c)
    {
        return CreateBitmap(c.ToString(), name, size, default, out measuredSize);
    }

This is a snippet of code from one of my projects, basically generateGlyph() is called for each character in a string
Sorry if there are any errors, I extracted this in a rush

To add one more thing, Rider's DPA no longer generates a ton of SOH allocation warnings

@JimBobSquarePants JimBobSquarePants merged commit 623498a into main Dec 16, 2024
15 checks passed
@JimBobSquarePants JimBobSquarePants deleted the js/hasflag-allocations branch December 16, 2024 04:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants