perf(ast): reduce size of Comment to 16 bytes

[camchenry]

I noticed that most of Comment consisted of enums which were only 1 byte in size, but only used a few bits in each byte. There are few enough fields that we can actually store all of them in a single u16 bit flag, which reduces the size of Comment from 24 bytes to 16 bytes.

[camchenry]

• perf(ast): reduce size of Comment to 16 bytes #11062 👈 (View in Graphite)
• main

---

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• 0-merge - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[codspeed-hq]

CodSpeed Instrumentation Performance Report

Merging #11062 will not alter performance

_{Comparing 05-15-perf_ast_reduce_size_of_comment_to_16_bytes (b9e51e2) with main (eef93b4)}

Summary

✅ 36 untouched benchmarks

[camchenry]

@overlookmotel I don't know if this conflicts too much with #11056, but I had started working on this a bit last night before I saw your PR. I think this should still be valid since this both removes a lot of the padding and also reduces the overall size of this struct. Not sure if it'll affect perf at all though, I'm just trying some things at this point.

I'm also expecting that this will break conformance, until I fix the ESTree serialization.

[overlookmotel]

We only need to lose 1 byte to get this type down to 16 bytes.

So rather than doing so much with the CommentFlags bitflag set, how about just combining preceded_by_newline and followed_by_newline into 1 byte?

That's simple enough that you could do it with an enum, and avoid complication of the bitflags! macro.

enum CommentNewlines {
    None,
    Leading,
    Trailing,
    LeadingAndTrailing,
}

Generated assembly is sometimes more efficient if a struct has no padding, so using all the available bytes can actually perform better than aggressively packing everything into the minimum number of bits. e.g. #11046 got a small perf gain from using more bits.

The other advantage is that preceded_by_newline and followed_by_newline are skipped in ESTree AST, so that avoids writing a custom ESTree serializer, which would be necessary with your current approach. I'm keen to avoid custom serializers as much as we can - they're a pain, and if the type changes later on, they have to be kept in sync.

[camchenry]

Generated assembly is sometimes more efficient if a struct has no padding, so using all the available bytes can actually perform better than aggressively packing everything into the minimum number of bits. e.g. #11046 got a small perf gain from using more bits.

Interesting! Seems worth a try.

The other advantage is that preceded_by_newline and followed_by_newline are skipped in ESTree AST, so that avoids writing a custom ESTree serializer, which would be necessary with your current approach. I'm keen to avoid custom serializers as much as we can - they're a pain, and if the type changes later on, they have to be kept in sync.

yeah I was just starting to figure out the serializer part. since we only need to lose 1 byte here, I'm leaning towards dropping the bitflags and keeping the enums, but combining them like you suggested. should hopefully make it a little easier to maintain too.

[overlookmotel]

I really hate the bitflags! macro! Half the time it doesn't do what we really need anyway.

Ideally we need an ergonomic way to pack enums together. e.g.:

pack_enums! {
    pub enum Foo {
        A,
        B,
        C,
        D,
    }

    pub enum Bar {
        E,
        F,
    }

    pub struct FooAndBar {
        pub foo: Foo,
        pub bar: Bar,
    }
}

pack_enums! would convert FooAndBar to something like:

pub struct FooAndBar(FooAndBarInner);

enum FooAndBarInner {
    A_and_E,
    B_and_E,
    C_and_E,
    D_and_E,
    A_and_F,
    B_and_F,
    C_and_F,
    D_and_F,
}

impl FooAndBar {
    pub fn foo(&self) -> Foo {
        match self.0 {
            Self::A_and_E | Self::A_and_F => Foo::A,
            Self::B_and_E | Self::B_and_F => Foo::B,
            Self::C_and_E | Self::B_and_F => Foo::C,
            Self::D_and_E | Self::B_and_F => Foo::D,
        }
    }

    pub fn bar(&self) -> Bar {
        match self.0 {
            Self::A_and_E | Self::B_and_E | Self::C_and_E | Self::D_and_E => Bar::E,
            Self::A_and_F | Self::B_and_F | Self::C_and_F | Self::D_and_F => Bar::F,
        }
    }
}

FooAndBar is now 1 byte, and has a niche so Option<FooAndBar> is 1 byte too.

I've not located a crate which does this, sadly.

[camchenry]

Looks like this gets us about the same result in codspeed (+1% in some lexer benchmarks). Running locally, I see a ~1% perf improvement in the parser (on my M1 Air laptop) (numbers are reversed, because I ran in the opposite branch order):

[overlookmotel]

Great!

Let's merge this.

However, my idea of using an enum instead of bitflags! for CommentNewlines was misjudged. I hadn't taken into account the complexity of the setters for the 2 flags. bitflags! would be simpler, and probably more performant - compiler is surprisingly bad at optimizing operations on fieldless enums.

Here's a comparison between the enum-based implementations and code like what bitflags! would produce: https://godbolt.org/z/qT817zo18

The most important ones to compare are the getters and the "real world usage" methods e.g. set_followed_by_newline_true.

So, although I do hate bitflags! it's still better than what I came up with!

I don't know if you have time/inclination to try making that change in a follow-up PR? Really sorry I sent you on a wild goose-chase with my bitflag-hating. You were on the right track to start with, and I led you astray...

[overlookmotel]

Pushed a commit to remove the change to the codegen for raw transfer. That change was I think left over from earlier version where used a u16 for flags.

[overlookmotel]

Merge activity

• May 15, 3:26 PM EDT: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• May 15, 3:30 PM EDT: overlookmotel added this pull request to the Graphite merge queue.
• May 15, 3:37 PM EDT: Merged by the Graphite merge queue.