refactor(syntax): turn the AstNodeId::dummy into a constant field.

[rzvxa]

No description provided.

[rzvxa]

• refactor(syntax): turn the AstNodeId::dummy into a constant field. #4308 👈
• main

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

Join @rzvxa and the rest of your teammates on Graphite

[codspeed-hq]

CodSpeed Performance Report

Merging #4308 will not alter performance

_{Comparing 07-16-perf_syntax_make_astnodeid_dummy_const (fc0b17d) with main (8e2fd8c)}

Summary

✅ 32 untouched benchmarks

[graphite-app]

Your org has enabled the Graphite merge queue for merging into main

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

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

[overlookmotel]

What's the theory? One would hope the compiler is smart enough to const fold it down to just 0 even without const.

Probably this is better than fn dummy:

impl AstNodeId {
    const DUMMY: AstNodeId = AstNodeId::from_raw_unchecked(0);
}

I should have done that originally when I added fn dummy. My bad.

[rzvxa]

What's the theory? One would hope the compiler is smart enough to const fold it down to just 0 even without const.

Probably this is better than fn dummy:

impl AstNodeId {
    const DUMMY: AstNodeId = AstNodeId::from_raw_unchecked(0);
}

I should have done that originally when I added fn dummy. My bad.

Well, I thought making it const would increase the performance by default but with const, it sometimes performs worse. After I saw the benchmark result I added that comment.

I did a few iterations on it(inline always, having dummy as global const, etc. however I squashed some of the commits), but I haven't tried this as a field. That could be the tipping point for the compiler to finally figure out it can be folded everywhere.

[overlookmotel]

I strongly suspect the compiler is const-folding it already, so we're not going to see any perf gain, no matter what we do. But DUMMY would be a more idiomatic way to do it.

[rzvxa]

I strongly suspect the compiler is const-folding it already, so we're not going to see any perf gain, no matter what we do. But DUMMY would be a more idiomatic way to do it.

I think you are right, It's the only thing that can explain the benchmark results.

[rzvxa]

Changed the PR category to the refactor since the perf gain is 0% all around😆, Weirdly enough constant function did worse than non-const, Maybe that was just the benchmark noise.

[overlookmotel]

Weirdly enough constant function did worse than non-const, Maybe that was just the benchmark noise.

Yes, I imagine it's noise. We've removed a lot of the variance in semantic's benchmarks (they used to swing around randomly by +/-3%) but still they're quite noisy. So it's very hard to take conclusions from them.

[graphite-app]

Merge activity

• Jul 16, 6:42 PM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Jul 16, 6:42 PM EDT: overlookmotel added this pull request to the Graphite merge queue.
• Jul 16, 6:46 PM EDT: overlookmotel merged this pull request with the Graphite merge queue.

[overlookmotel]

Thanks for trying this. Sorry it didn't yield the wild perf boost of your dreams!

[overlookmotel]

Omg found the reason behind no perf gain, We don't use the max check so they are basically the same(with some const casts in between).

            /// Asserts `v <= Self::MAX_INDEX` unless Self::CHECKS_MAX_INDEX is false.
            #[inline]
            $v fn check_index(v: usize) {
                if Self::CHECKS_MAX_INDEX && (v > Self::MAX_INDEX) {
                    $crate::__max_check_fail(v, Self::MAX_INDEX);
                }
            }

I'm not familiar with what define_index_type! does. Are you saying that we have Self::CHECKS_MAX_INDEX = false, so indexes aren't checked? Is this unsound? Or does it "just" risk ID wrapping around back to 0, so not undefined behavior but likely will lead to incorrect results?

Regardless, I doubt that's the reason for no perf change. Even with CHECKS_MAX_INDEX enabled, compiler should be able to const fold - it's not hard to prove that (0 > Self::MAX_INDEX) == false.

[rzvxa]

No, I was desperate to find an explanation and that wasn't the case. Max checks are on by default but they are both const.

Regardless, I doubt that's the reason for no perf change. Even with CHECKS_MAX_INDEX enabled, compiler should be able to const fold - it's not hard to prove that (0 > Self::MAX_INDEX) == false.

Exactly; This was the reason behind it, please disregard my previous comment.