Value: box BigDecimal and Array

[tyt2y3]

These two types are already internally heap-allocated.

And boxing these two would bring down Value to 32 bytes.

I hope this is a fair trade off.

[tyt2y3]

@Expurple @Huliiiiii

[Expurple]

I disagree that going 40->32 bytes is worth verbose user-visible boxing. What's even the reason for doing that? You don't include any benchmark results in the PR description.

My colleagues are always frustrated with SeaORM when they have to construct expressions like Value::Array(ArrayType::BigInt, Some(Box::new(foo))). Even the diff shows that the code is more verbose

[tyt2y3]

32 is a magic power-of-two number that's more cache line friendly. such that most value variants don't get pessemisied by Array and BigInt. they are using heap allocation anyway, so the penalty is relatively smaller.

if you are concerned about ergonomics, we can add a few From impl.

[Expurple]

Yeah, From impls need attention, regardless of boxing. I have some personal TODOs regarding those painful conversions into Value. I'll sort it out and publish later.

32 is a magic power-of-two number that's more cache line friendly.

Sure, but how important is that in practice? I could only find a single open performance-related issue across SeaORM and SeaQuery repos: #259. On the other hand, ergonomics has always been a big issue for me and my team.

I'm vary of micro-optimizing without representative benchmarks. And I doubt that Values are moved or accessed so often that it dominates a separate allocation/deallocation for every Value (of a boxed variant). I'd assume that, most of the time, Values are constucted close to the query, then moved into some internal Vec, then sequentially iterated for serialization, and that's it. Cache lines shouldn't matter for sequential iteration over a Vec, due to pre-fetching.

[Huliiiiii]

Btw, the size of Json is also 40, so boxing them won’t make Value smaller.

[tyt2y3]

https://play.rust-lang.org/?version=stable&mode=debug&edition=2024

Btw, the size of Json is also 40, so boxing them won’t make Value smaller.

it's 32. it's interesting that it has enough unused bits that it's still 32 after niche field optimization.

ergonomics has always been a big issue

I think a Value::array<T>() method would solve the issue, then ArrayType don't have to be manually set.

just to say I am not against 40, but inflating it to 40 here brings no additional benefit.

the gain from 24 -> 32 is well worth the memory.

may be we add a few more variants then it becomes 40 inevitably.

[Expurple]

Ok, let's move on. At least, now we have a consistent, documented, tested, and benchmarked approach

[tyt2y3]

Big thanks to both of you! @Huliiiiii @Expurple

[Huliiiiii]

Oh, enabling both "with-json" and "with-bigdecimal" breaks niche optimizations.

I agree with Expurple's point — Value is more often used inside a Vec, and the overhead from double heap allocation may not be smaller than the benefit gained from size optimization.

I’ve updated the benchmarks — for inputs with 10 BigDecimal values, performance dropped by about 5–10%. The reduced size of Value didn’t improve the performance of unboxed variants.