performance[vortex-array]: don't call is_valid to count bytes in varbinview

[asubiotto]

I noticed that the better blocks compressor uses count_referenced_bytes which calls is_valid on each view and results in an expensive scalar_at call. This was 30% of our system-wide CPU usage over 12 hours (see PR for attached screenshot).

This commit iterates over the nullability mask directly to avoid these expensive calls.

[codspeed-hq]

CodSpeed Performance Report

Merging #5814 will improve performance by 17.98%

_{Comparing asubiotto/viewcount (7fda16d) with develop (fdd2fcc)}

Summary

⚡ 4 improvements
✅ 1250 untouched
⏩ 623 skipped¹

Benchmarks breakdown

	Benchmark	BASE	HEAD	Efficiency
⚡	compact_sliced[(4096, 90)]	547.8 µs	466 µs	+17.56%
⚡	compact_sliced[(16384, 90)]	2.1 ms	1.8 ms	+17.98%
⚡	compact_sliced[(4096, 10)]	641.4 µs	559.6 µs	+14.61%
⚡	compact_sliced[(16384, 10)]	2.5 ms	2.2 ms	+14.98%

Footnotes

1. 623 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.93%. Comparing base (265f29a) to head (7fda16d).
⚠️ Report is 3 commits behind head on develop.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[AdamGS]

compression slowdown is interesting, I'll a look but overall seems like a win 🥳

[AdamGS]

LGTM

[asubiotto]

It seems like it's due to now including runend_encode. This is used in the decision to compress, so it might be compressing when previously it wasn't? I'll double check since the behavior shouldn't have changed.

[AdamGS]

Looking at a few codspeed diffs, seems like it can't figure out the diff correctly, so it thinks all the traces are new

[asubiotto]

Looking at a few codspeed diffs, seems like it can't figure out the diff correctly, so it thinks all the traces are new

Yes, I also think this is codspeed. I don't think the runend benchmarks (that the regression reported) use varbinviews. Also now codspeed seems to be happy again.

[robert3005]

I am very confused how VarBinViewVTable::is_valid ends up calling scalar at, is it the case that you have runend compressed validity?

[asubiotto]

is_valid seems to always call scalar_at:

vortex/vortex-array/src/validity.rs

Line 135 in 31026ce

let scalar = a.scalar_at(index);

But it does seem like the expensive part is specifically scalar_at on a RunEndVTable which implies validity is REE encoded. We use the default write strategy.

[robert3005]

Ah, it was me not reading through the code deep enough.

[gatesn]

Yeah this is why I want to make it really really obvious with the operators work what is "executing", vs operating on metadata only. I think even deprecating these scalar validity accessor functions, and instead just forcing the user to execute the array validity. (Still working on a name for "execute"!)

Great catch though