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

Proposal: Use integers for FontStyle #6

Closed
joachimschmidt557 opened this issue Aug 15, 2020 · 8 comments
Closed

Proposal: Use integers for FontStyle #6

joachimschmidt557 opened this issue Aug 15, 2020 · 8 comments

Comments

@joachimschmidt557
Copy link
Member

Currently, FontStyle is represented as a (possibly packed after #5) struct of bools.

I think using integer types and bitwise operations, we could achieve better and possibly faster functionality. Consider the following simplified example:

const FontStyle = u3;

const default: FontStyle = 0b000;
const bold: FontStyle = 0b001;
const dim: FontStyle = 0b010;
const italic: FontStyle = 0b100;

Combining font styles can be done with bitwise or; checking for a style can be done with bitwise and.

cc @data-man

@data-man
Copy link
Contributor

I think bitwise operations will decrease output's performance.

@joachimschmidt557
Copy link
Member Author

@data-man That may be right for checking a specific field of the FontStyle, i.e. checking whether font_style.bold == true. When using integers, we have an additional overhead of one bitwise operation in addition to the comparison.

But for other purposes, this has increased performance:

  • checking if multiple fields are set
  • checking for equality (includes checking isDefault)
  • checking if a given font style has at least the attributes of another font style (no attributes were removed)

All operations are turned into O(1) operations instead of O(n) where n is the number of fields.

@data-man
Copy link
Contributor

When using integers, we have an additional overhead of one bitwise operation in addition to the comparison.

And for convenience, some helpers will be needed: isBold/setBold, etc.

@joachimschmidt557
Copy link
Member Author

@data-man Yes, that would aid the usability of FontStyle if it would be represented as an integer. But those will be small functions which will be almost-surely inlined in a release build, so the overhead of an additional function call won't be there, only the overhead of the bitwise operation then.

I'll leave this proposal open until some benchmarks have been integrated into the library. From then on, we can test the performance of checking attributes/setting attributes/checking equality/subset of attributes/etc. And from these results, we can then proceed to select the appropriate representation of FontStyle

@data-man
Copy link
Contributor

@joachimschmidt557
I think helpers for RGB type are more important. Maybe in new module.
Rust's rgb24 as example.

@joachimschmidt557
Copy link
Member Author

@data-man If you want, feel free to implement that.

@data-man
Copy link
Contributor

pub const FontStyle = packed struct {
...
const Self = @This();
...
pub fn asUInt(self: Self) u11 {
    return @bitCast(u11, self);
}
...

?

@joachimschmidt557
Copy link
Member Author

@data-man Yup, don't know why this hasn't occurred to me yet. This means we can get the best of both worlds. I guess we can close this issue now.

This issue was closed.
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

No branches or pull requests

2 participants