refactor(array): use into for append datum matching [IGNORE ME]

[BugenZhao]

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

As suggested at #9113 (comment), this reduces the domain of match and may improve the performance (?).

Checklist For Contributors

• I have written necessary rustdoc comments
• I have added necessary unit tests and integration tests
• I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
• I have demonstrated that backward compatibility is not broken by breaking changes and created issues to track deprecated features to be removed in the future. (Please refer to the issue)
• All checks passed in ./risedev check (or alias, ./risedev c)

Checklist For Reviewers

• I have requested macro/micro-benchmarks as this PR can affect performance substantially, and the results are shown.

Documentation

• My PR DOES NOT contain user-facing changes.

Click here for Documentation

Types of user-facing changes

Please keep the types that apply to your changes, and remove the others.

• Installation and deployment
• Connector (sources & sinks)
• SQL commands, functions, and operators
• RisingWave cluster configuration changes
• Other (please specify in the release note below)

Release note

[kwannoel]

Does into also result in O(n) lookup?

[BugenZhao]

I'm not sure about the compiler's optimization. 🥺 Let's wait for the flamegraph.

[codecov]

Codecov Report

Merging #9527 (cba457c) into main (5fabd2a) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #9527      +/-   ##
==========================================
+ Coverage   70.72%   70.80%   +0.07%     
==========================================
  Files        1232     1232              
  Lines      206505   206500       -5     
==========================================
+ Hits       146055   146204     +149     
+ Misses      60450    60296     -154     

Flag	Coverage Δ
rust	70.80% <100.00%> (+0.07%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/common/src/array/mod.rs	74.36% <100.00%> (+0.93%)	⬆️

... and 6 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[lmatz]

[kwannoel]

Today's main-cron:

[BugenZhao]

Seems no obvious improvements. 😟

[lmatz]

It seems inevitable to touch SourceStreamChunkRowWriter 🥵

[kwannoel]

Maybe can try union also?

dyn_no_info             time:   [198.01 ns 198.49 ns 198.98 ns]
                        change: [-1.2664% -0.7003% -0.1354%] (p = 0.01 < 0.05)
                        Change within noise threshold.
Found 3 outliers among 100 measurements (3.00%)
  2 (2.00%) high mild
  1 (1.00%) high severe

union dyn               time:   [150.13 ns 150.60 ns 151.08 ns]
                        change: [-0.0452% +0.7195% +1.4891%] (p = 0.08 > 0.05)
                        No change in performance detected.
Found 2 outliers among 100 measurements (2.00%)
  1 (1.00%) high mild
  1 (1.00%) high severe

Did an experiment with union comparing these:

https://github.com/kwannoel/rust-experiments/blob/fb7a604bc7ab364ca21a60a1ea808dcf6779dab1/benches/match-dyn.rs#L207-L214

    fn append_u(&mut self, d: DynE, x: DynU) {
         match d {
           DynE::A => { unsafe { self.a.append_u(x) } },
           DynE::B => { unsafe { self.b.append_u(x) } },
           DynE::C => { unsafe { self.c.append_u(x) } },
         }
    }

https://github.com/kwannoel/rust-experiments/blob/fb7a604bc7ab364ca21a60a1ea808dcf6779dab1/benches/match-dyn.rs#L137-L144

    fn append_no_info(&mut self, x: Dyn) {
        match (self, x) {
            (Dyns::A(xs), Dyn::A(x)) => { xs.append(x) },
            (Dyns::B(xs), Dyn::B(x)) => { xs.append(x) },
            (Dyns::C(xs), Dyn::C(x)) => { xs.append(x) },
            (_, _) => { panic!("failed...") }
        }
    }

Seems like with union (first one), we can get 25% improvement. Trade-off is using unsafe code, and perhaps making the code more complex.

[BugenZhao]

Maybe can try union also?

It seems that std::hint::unreachable_unchecked on enum matching has the same effect as union. 🤔 I'd prefer to avoid introducing unsafe structures for this.

Comparison