Skip to content

Conversation

@Centril
Copy link
Contributor

@Centril Centril commented Apr 8, 2024

Description of Changes

This primarily shrinks the size of types AlgebraicValue from 32 bytes to 24 and AlgebraicType from 32 to 16.
The size of some other types are also shrunk, notably, table, column, index, sequence, etc. definitions are shrunk by moving from String to Box<str>. Moreover, ProductValue is shrunk to 16 bytes, a type that is used frequently in hot places.

Benchmarks below relative to based master, run on i7-7700K, 64GB.

full-scan               time:   [80.111 ms 80.529 ms 80.979 ms]
                        change: [-14.103% -13.427% -12.763%] (p = 0.00 < 0.05)
                        Performance has improved.

Benchmarking full-join: Collecting 100 samples in estimated 6.
full-join               time:   [301.14 µs 302.01 µs 303.25 µs]
                        change: [-9.6948% -9.1117% -8.5214%] (p = 0.00 < 0.05)
                        Performance has improved.

Benchmarking incr-select: Collecting 100 samples in estimated 
incr-select             time:   [183.21 ns 185.12 ns 188.14 ns]
                        change: [-5.7687% -4.8383% -3.6110%] (p = 0.00 < 0.05)
                        Performance has improved.

API and ABI breaking changes

None

Expected complexity level and risk

2

Testing

No semantic changes, existing tests should suffice.

@Centril Centril force-pushed the centril/shrink-basic branch 4 times, most recently from b4ffa79 to 218e64e Compare April 8, 2024 22:35
@Centril Centril changed the base branch from master to centril/isj-less-work April 8, 2024 22:42
@Centril Centril requested a review from kazimuth April 8, 2024 22:43
@Centril Centril marked this pull request as ready for review April 8, 2024 22:43
@Centril Centril requested a review from cloutiertyler as a code owner April 8, 2024 22:43
Copy link
Contributor

@kazimuth kazimuth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice. I see why this took longer than expected, this is the sort of thing that gets everywhere.

My only real concern right now would be that this could result in $O(n^2)$ behavior for loops that repeatedly append to ArrayValues. I'm not aware of such loops off the top of my head, and I don't see any methods changed in this PR that expose such an interface for ArrayValue, so I don't think it's a problem.

///
/// We box these up as they allow us to shrink `AlgebraicValue`.
U128(u128),
U128(Packed<u128>),
Copy link
Contributor

@kazimuth kazimuth Apr 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may inhibit SIMD optimizations for i/u128. Fortunately, that doesn't matter in the slightest -- packing AV and AT is way more important.

Base automatically changed from centril/isj-less-work to master April 9, 2024 00:09
@Centril Centril force-pushed the centril/shrink-basic branch 2 times, most recently from 70b4e57 to b71e8c1 Compare April 9, 2024 00:52
Copy link
Contributor

@cloutiertyler cloutiertyler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine in principle, but let's be deferential to higher prio things about merging this.

@Centril Centril force-pushed the centril/shrink-basic branch from b71e8c1 to b27314a Compare April 13, 2024 16:33
@Centril
Copy link
Contributor Author

Centril commented Apr 13, 2024

Commitlog has merged, so going ahead and merging this.

@Centril Centril enabled auto-merge April 13, 2024 16:35
@Centril Centril added this pull request to the merge queue Apr 13, 2024
Merged via the queue into master with commit d6815eb Apr 13, 2024
@Centril Centril deleted the centril/shrink-basic branch April 13, 2024 17:10
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

Successfully merging this pull request may close these issues.

4 participants