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

Reducing size of Color #31

Closed
pickfire opened this issue Jul 31, 2020 · 2 comments · Fixed by #67
Closed

Reducing size of Color #31

pickfire opened this issue Jul 31, 2020 · 2 comments · Fixed by #67

Comments

@pickfire
Copy link
Contributor

pickfire commented Jul 31, 2020

I want to give a try to reduce size of Color. Currently,

error: size: Size { raw: 216 }
  --> src/color/mod.rs:32:1
   |
32 | / pub(crate) struct Color {
33 | |     rgba: Rgba,
34 | |     hsla: Option<Hsla>,
35 | |     repr: String,
36 | | }
   | |_^

error: size: Size { raw: 96 }
  --> src/color/mod.rs:79:1
   |
79 | / struct Rgba {
80 | |     red: Number,
81 | |     green: Number,
82 | |     blue: Number,
83 | |     alpha: Number,
84 | | }
   | |_^

error: size: Size { raw: 32 }
  --> src/color/mod.rs:88:1
   |
88 | / struct Rgba2 {
89 | |     red: f64,
90 | |     green: f64,
91 | |     blue: f64,
92 | |     alpha: f64,
93 | | }
   | |_^

error: size: Size { raw: 24 }
  --> src/value/number/mod.rs:21:1
   |
21 | / pub(crate) enum Number {
22 | |     Small(Rational64),
23 | |     Big(Box<BigRational>),
24 | | }
   | |_^

Currently,

pub(crate) struct Color {
    rgba: Rgba,
    hsla: Option<Hsla>,
    repr: String,
}

struct Rgba {
    red: Number,
    green: Number,
    blue: Number,
    alpha: Number,
}

struct Hsla {
    hue: Number,
    saturation: Number,
    luminance: Number,
    alpha: Number,
}

It looks quite huge. I wonder if the following is possible.

pub(crate) struct Color {
    rgb: Rgb,
    // Do we really need an Option since usually I see either one of Rgba or Hsla can be specified?
    // Or can both be specified at the same time?
    // Or maybe just convert Hsla to Rgba and store only Rgba? Not sure if this works.
    hsl: Option<Hsl>,
    // I think we could probably follow libsass and use double precision here.
    alpha: f64,
    // I suggest using SmartString later on for this to allow small string optimization (23 bits could be stored in stack IIRC)
    // I wonder if it would be beneficial if this can be lazily calculated without keeping it in memory?
    repr: SmartString,
}

struct Rgb {
    // Since we are converting these to u8 later on, do we really need it to be Ratio or we can use u8?
    // Or is it used for those translate functions, I am not sure.
    // If u8 is not possible, I guess f64 would be a better choice.
    red: u8,
    green: u8,
    blue: u8,
}

// Have yet checked much on this yet, I bet u8 is not possible here.
struct Hsl {
    hue: f64,
    saturation: f64,
    luminance: f64,
}

This could probably get us to 64 bytes from 216. Or if we split out Hsla from a pointer (Box), we could probably get 32 bytes. I personally think it could be beneficial for cache lines to speed things up, not sure how true is this but we saw performance improvements last time for size reduction IIRC.

@connorskees What do you think?

@connorskees
Copy link
Owner

The code that handles colors is indeed some of the oldest and in a deep need of refactoring. If I remember correctly, it is necessary to store hsl alongside rgb due to the precision loss when converting hsl colors to rgb. I believe even with arbitrary precision, the conversion from hsl to rgb back to hsl will still be off somewhat. Something like hue(hsl(4.5678901, 1, 1)) should return 4.5678901.

rgb is calculated eagerly I believe just to make things easier, under the assumption that colors are emitted as rgb either way.

I will need to look into the dart-sass implementation a bit more to figure out how they store colors.

@pickfire
Copy link
Contributor Author

pickfire commented Aug 1, 2020

dart-sass keeps all of them https://github.com/sass/dart-sass/blob/6565b45a6ce17380839af3ce166365595a1fbdc9/lib/src/value/color.dart#L16-L56 but I believe we can calculate it lazily without storing it. libsass on the other hand only store rgba for rgba color and hsla for hsla color, we could follow this. So I say we can store enum { colorkind: Rgb | Box<Hsl> (reduce the size if Rgb uses u8), alpha: f64, s: SmartString }. If we need hsl we can calculate the value from rgb.

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 a pull request may close this issue.

2 participants