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

Switch from arrow2 to arrow-rs since arrow2 has been deprecated #68

Open
NickCondron opened this issue Jul 12, 2024 · 5 comments
Open

Comments

@NickCondron
Copy link
Contributor

jorgecarleitao/arrow2#1429

@hohav
Copy link
Owner

hohav commented Jul 20, 2024

Thanks for raising this. I looked into it a while back, and determined that it was going to be a nontrivial amount of work for no immediate gain. I guess we'll eventually have to switch, but I'm just not very motivated to slog through it at the moment.

Maybe the first step would be to note the salient differences between the two libraries. Help would be welcome there.

@NickCondron
Copy link
Contributor Author

I took an initial crack at it. Here are notable differences so far:

  • MutablePrimitiveArray<i32> becomes PrimitiveBuilder<Int32Type>
  • PrimitiveArray<u32> becomes PrimitiveArray<UInt32Type>
  • When constructing your own validity you can use the NullBufferBuilder type. It finalizes to Option<NullBuffer> only allocating the buffer if there is at least one null. This simplifies some of the repetitive logic for this that is currently in peppi.
  • There's less guardrails and convenience for the Offsets array (called OffsetBuffer in arrow-rs)

I haven't done much, but what I've done so far might help illustrate some of these: ef50bf2

@hohav
Copy link
Owner

hohav commented Sep 8, 2024

@NickCondron Thanks for the head start! I have this working now in the arrow-rs2 branch. It relies on a hack to work around a bug in arrow-rs— I'm planning to wait until this PR lands so I can remove the hack.

There's also a couple of performance regressions that need investigation:

  1. Our criterion benchmarks (which I'm really happy to have BTW) all regressed on my M2 Mac, mostly by around 20-25%.
  2. The output from io::peppi::write got larger by about 5%, despite containing the same information (i.e. round-trips successfully).

My initial guess on 2 is that we might be unnecessarily storing some all-1 validity bitmaps, but I haven't figured out how to check that yet. No idea on 1 yet, and I don't know when I'll have the time for that kind of deep dive. Help would be very welcome!

@NickCondron
Copy link
Contributor Author

Here are my (similar) performance numbers for my x86 linux machine:

into_game/old_ver_thegang.slp
                        time:   [6.8254 ms 6.8272 ms 6.8296 ms]
                        change: [+3.7968% +4.1083% +4.4299%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 4 outliers among 100 measurements (4.00%)
  1 (1.00%) high mild
  3 (3.00%) high severe

into_game/hbox_llod_timeout_g8.slp
                        time:   [19.372 ms 19.397 ms 19.423 ms]
                        change: [+10.778% +11.002% +11.203%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 23 outliers among 100 measurements (23.00%)
  10 (10.00%) low severe
  3 (3.00%) low mild
  2 (2.00%) high mild
  8 (8.00%) high severe

Benchmarking into_game/casual_doubles.slp: Warming up for 1.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 8.1s, or reduce sample count to 60.
into_game/casual_doubles.slp
                        time:   [78.710 ms 79.032 ms 79.329 ms]
                        change: [+11.833% +12.490% +13.028%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 9 outliers among 100 measurements (9.00%)
  5 (5.00%) low severe
  3 (3.00%) high mild
  1 (1.00%) high severe

into_game/short_game_tbh10.slp
                        time:   [84.170 µs 84.372 µs 84.575 µs]
                        change: [+21.368% +21.594% +21.788%] (p = 0.00 < 0.05)
                        Performance has regressed.

into_game/ics_ditto.slp time:   [12.191 ms 12.193 ms 12.195 ms]
                        change: [+13.029% +13.065% +13.100%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 7 outliers among 100 measurements (7.00%)
  1 (1.00%) low mild
  3 (3.00%) high mild
  3 (3.00%) high severe

into_game/mango_zain_netplay.slp
                        time:   [10.090 ms 10.097 ms 10.106 ms]
                        change: [+14.428% +14.574% +14.706%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 10 outliers among 100 measurements (10.00%)
  1 (1.00%) low mild
  5 (5.00%) high mild
  4 (4.00%) high severe

skip_frames/old_ver_thegang.slp
                        time:   [7.7803 µs 7.7921 µs 7.8080 µs]
                        change: [+39.461% +40.069% +40.651%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 5 outliers among 100 measurements (5.00%)
  2 (2.00%) high mild
  3 (3.00%) high severe

skip_frames/hbox_llod_timeout_g8.slp
                        time:   [11.926 µs 11.943 µs 11.977 µs]
                        change: [+62.008% +62.486% +62.964%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 16 outliers among 100 measurements (16.00%)
  5 (5.00%) high mild
  11 (11.00%) high severe

skip_frames/casual_doubles.slp
                        time:   [22.375 µs 22.390 µs 22.407 µs]
                        change: [+58.557% +59.171% +59.726%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 10 outliers among 100 measurements (10.00%)
  6 (6.00%) high mild
  4 (4.00%) high severe

skip_frames/short_game_tbh10.slp
                        time:   [11.916 µs 11.922 µs 11.929 µs]
                        change: [+61.450% +61.906% +62.347%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 14 outliers among 100 measurements (14.00%)
  7 (7.00%) high mild
  7 (7.00%) high severe

skip_frames/ics_ditto.slp
                        time:   [15.840 µs 15.845 µs 15.851 µs]
                        change: [+68.302% +68.694% +69.040%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 14 outliers among 100 measurements (14.00%)
  6 (6.00%) high mild
  8 (8.00%) high severe

skip_frames/mango_zain_netplay.slp
                        time:   [13.515 µs 13.524 µs 13.537 µs]
                        change: [+54.581% +55.082% +55.532%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 19 outliers among 100 measurements (19.00%)
  5 (5.00%) high mild
  14 (14.00%) high severe

The surprising thing is the dramatic increase in the skip_frames tests which shouldn't use anything arrow related. If you can think of anything that could have caused this, let me know. I will continue to investigate.

@hohav
Copy link
Owner

hohav commented Sep 30, 2024

I'm less concerned about skip_frames because it's already so fast that we're only talking about a few microseconds. I care much more about how long it takes to parse the frame data.

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