Use zero-copy Arrays in `--release` #1116

workingjubilee · 2023-04-20T01:37:53Z

This introduces a pile of "array-walking" code to implement truly "zero-copy" arrays... in release mode. It may make tests notably slower for a bit, because it includes a pile of assertions. Afterwards, a lot of the code, especially the assertion code, can be pared back, simplified, or changed into less high-overhead forms once this has been tested to our satisfaction. Ultimately, I prioritized correctness against the previous behavior of our library rather than validating against Postgres per se (because it's not as clear that our new behavior will correctly model Postgres).

There are no external, user-facing API changes required to take advantage of this. In the future we may wish to consider such, as there are further improvements atop this in terms of correctness and performance to be had.

workingjubilee · 2023-04-20T01:54:51Z

Also to be clear: the new array-walking code is used unconditionally, it's merely that during tests we lug around ye olde "big slice of datums" to assert against (thus making it not "zero-copy"), because I couldn't think of a different way without publicly exposing functions I would rather not.

workingjubilee · 2023-04-20T02:59:00Z

pgrx/src/datum/array.rs

+            // SAFETY: The constructor for this type instantly panics if any nulls are present!
+            // Thus as an invariant, this will never have to reckon with the nullbitmap.
+            let element = unsafe { array.bring_it_back_now(*ptr, *curr, false) };
+            *curr += 1;
+            *ptr = unsafe { array.one_hop_this_time(*ptr, array.elem_layout) };
+            element


I almost forgot about this one!

We might as well exploit the invariant that this is now an instantly panicking constructor (and has been for a while, actually). That said, because of the way that the internal NullKind type works (where it embeds, on construction, some knowledge about whether the array can have a null), I honestly expect it is not going to be a big improvement that often.

eeeebbbbrrrr

This is really nice. Really really nice.

pgrx/src/datum/array.rs

eeeebbbbrrrr · 2023-04-20T14:26:30Z

pgrx/src/datum/array.rs

-        ptr.unwrap_or(ptr::null())
+    /// Rips out the underlying `pg_sys::ArrayType` pointer.
+    /// Note that Array may have caused Postgres to allocate to unbox the datum,
+    /// and this can hypothetically cause a memory leak if so.


I don't see the hypothetical memory leak here?

Is it that Array.raw might be detoasted (ie, allocated) and here we are giving the pointer away?

Is that considered a memory leak? I'm not so sure.

Yeah, that's the only thing I can think of. I can delete that remark if it's confusing!

Maybe this is a little more subtle than we think? If we give the pointer back to the user, it becomes their responsibility to free it (or to let it dangle until Postgres does garbage collection, which is probably the right answer), but there's no way for the user to know if it could safely be freed.

Could we just remove this function entirely? I don't see a burning need to give out an ArrayType pointer, do you?

One user uses this to go from Array to RawArray, I believe.

eeeebbbbrrrr · 2023-04-20T14:36:34Z

pgrx/src/datum/array.rs

-            self.curr += 1;
-            Some(element)
+        let Self { array, curr, ptr } = self;
+        let is_null = array.null_slice.get(*curr)?;


heh, that ? is subtle. Took me a minute to figure out how this function would ever return None.

Also, this code is duplicated down below at line 454... is there room to consolidate the two iterators? I can't see any difference between them.

To the first comment, I used let-else to make all the load-bearing ? more obvious.

To the second comment, mostly, I tried to avoid any breaking changes to the API, and currently the API does yield two distinct types of iterators.

To the second comment, mostly, I tried to avoid any breaking changes to the API, and currently the API does yield two distinct types of iterators.

That's fine for now but we ought to clean it up at some point.

eeeebbbbrrrr · 2023-04-20T14:40:31Z

pgrx/src/datum/array.rs

+            };
+
+            // SAFETY: ptr stops at 1-past-end of the array's varlena
+            debug_assert!(ptr.wrapping_add(offset) <= self.raw.end_ptr());


pgrx/src/datum/array.rs

eeeebbbbrrrr · 2023-04-20T15:05:33Z

Also to be clear: the new array-walking code is used unconditionally, it's merely that during tests we lug around ye olde "big slice of datums" to assert against (thus making it not "zero-copy"), because I couldn't think of a different way without publicly exposing functions I would rather not.

Yeah, I wonder if we should ditch that entirely?

eeeebbbbrrrr · 2023-04-20T17:13:17Z

How does this handle Postgres multi-dimensional arrays? I don't suppose it does?

I'm not suggesting we make that part of this work, but I guess we need to make sure this doesn't blow up if given a multidimensional array...

workingjubilee · 2023-04-20T17:30:32Z

Postgres, like everyone else, does a linearizing transform of the multidimensional space, because ultimately it must, as memory on a "flat memory model" processor is a 1-dimensional space, described only by the number line. So this doesn't make us handle multidimensional arrays, but it does allow us to successfully walk a linear array, and Postgres handles multidimensional arrays as a slight embroidery on the handling of linear arrays.

With all of these pieces, PGX can finally disassemble Array at will with minimal Drop fussing, as Array is no longer managing its own Drop! To preserve low-level functions such as `Array::into_array_type`, ManuallyDrop can inhibit the `Toast<RawArray>` from pfreeing itself, so use that but document the leakage for users.

These debug asserts validate correctness of our varlena realignment code. Note that the `Size::CStr` and `Size::Fixed, PassBy::Ref` branches unfortunately seem to be underexercised at the moment by our tests.

Just factors out the array walking code in a reasonable form, with slightly less optimizations, but also less duplicate code.

There are larger gains to be had in the future, but this is a reasonable start on this.

Also in general tighten up everything, less doublechecking lens, et cetera. This also requires correctly checking indices into null_slice as less-than, not less-or-equal to the length!

Array's null_slice field safely wraps the nullbitmap. All of its uses justify its presence, so remove plan to drop it.

let-else is more blatant than a single question mark, and these are all cases where the ? does a lot of work.

workingjubilee · 2023-04-20T18:02:42Z

I rebased this on develop.

pgrx/src/toast.rs

workingjubilee · 2023-04-20T18:54:07Z

Postgres, like everyone else, does a linearizing transform of the multidimensional space, because ultimately it must, as memory on a "flat memory model" processor is a 1-dimensional space, described only by the number line.

these are really fancy words that mean "yeah uh a sufficiently large 1D array and an appropriately sized 2D array look the same in memory".

workingjubilee · 2023-04-20T22:07:21Z

This final commit is an assertion that is correctness-critical and cannot be removed. I realized it was implicit while rereading the code and describing it to someone who asked me about it. If it proves wrong, we can do the same alignment fixups as for varlenas for fixed-width types (in fact, we can do a single fixup once). I am pushing an assertion instead of simply fixing the code because I don't want to believe that Postgres would use an unaligned typlen like this.

eeeebbbbrrrr · 2023-04-20T22:09:47Z

This final commit is an assertion that is correctness-critical and cannot be removed. I realized it was implicit while rereading the code and describing it to someone who asked me about it. If it proves wrong, we can do the same alignment fixups as for varlenas for fixed-width types (in fact, we can do a single fixup once). I am pushing an assertion instead of simply fixing the code because I don't want to believe that Postgres would use an unaligned typlen like this.

woah

workingjubilee requested a review from eeeebbbbrrrr April 20, 2023 01:37

workingjubilee commented Apr 20, 2023

View reviewed changes

eeeebbbbrrrr reviewed Apr 20, 2023

View reviewed changes

workingjubilee added 22 commits April 20, 2023 10:39

Add Toasty and Toast

736d21b

impl Toasty for RawArray

819ecb3

Comment on into_array_type

ca4b964

Unoptimized zero copy array code

6056890

Zero-copy for null-free fixed-width arrays

0b5e868

Zero-copy for all array kinds

0181a4e

Comment on Postgres realign code

40e65c2

Actually stop deconstructing

7be91e9

debug_assert! in Array::walk_index

72b48e7

These debug asserts validate correctness of our varlena realignment code. Note that the `Size::CStr` and `Size::Fixed, PassBy::Ref` branches unfortunately seem to be underexercised at the moment by our tests.

Clean up datum::Array code

556cae9

Just factors out the array walking code in a reasonable form, with slightly less optimizations, but also less duplicate code.

Clean up release build warnings

b1a1a17

Optimize zero-copy iterator impls

e238c73

There are larger gains to be had in the future, but this is a reasonable start on this.

inline walk_index into get

3606a24

Also in general tighten up everything, less doublechecking lens, et cetera. This also requires correctly checking indices into null_slice as less-than, not less-or-equal to the length!

Document safety reqs of internal array fn

91a1f01

Document more fine details

5dc239c

Err on the side of caution while offsetting

bf0128d

debug assert for not exceeding the varlena

b850d18

Remove unnecessary unsafe

d23a06e

Array's null_slice field safely wraps the nullbitmap. All of its uses justify its presence, so remove plan to drop it.

Adjust array test formatting

bed66fb

Finish the iteration opt for ArrayTypedIterator

1d489dc

Array: Make load-bearing ? more obvious

aab091f

let-else is more blatant than a single question mark, and these are all cases where the ? does a lot of work.

workingjubilee force-pushed the joy/zero-copy-arrays branch from 5df479a to cc219bf Compare April 20, 2023 18:02

workingjubilee added 2 commits April 20, 2023 11:14

pfree bools ptr in Array::deconstruct_from

b9d4e57

Add comment on the byte ptr

4ba1369

workingjubilee force-pushed the joy/zero-copy-arrays branch from cc219bf to 4ba1369 Compare April 20, 2023 18:15

eeeebbbbrrrr reviewed Apr 20, 2023

View reviewed changes

pgrx/src/toast.rs Outdated Show resolved Hide resolved

Toasty::drop_toast is unsafe

adb2a6e

eeeebbbbrrrr approved these changes Apr 20, 2023

View reviewed changes

workingjubilee mentioned this pull request Apr 20, 2023

Truly zero-copy arrays #1118

Open

10 tasks

Assert unpadded fixed-width layouts

b717020

workingjubilee merged commit 13c4eb7 into develop Apr 20, 2023

workingjubilee mentioned this pull request Apr 21, 2023

No needless palloc in Array<'a, T> #880

Closed

eeeebbbbrrrr mentioned this pull request Apr 24, 2023

Update version to 0.8.0 #1121

Merged

workingjubilee deleted the joy/zero-copy-arrays branch June 20, 2023 17:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use zero-copy Arrays in `--release` #1116

Use zero-copy Arrays in `--release` #1116

workingjubilee commented Apr 20, 2023 •

edited

Loading

workingjubilee commented Apr 20, 2023 •

edited

Loading

workingjubilee Apr 20, 2023

eeeebbbbrrrr left a comment

eeeebbbbrrrr Apr 20, 2023

eeeebbbbrrrr Apr 20, 2023

workingjubilee Apr 20, 2023

eeeebbbbrrrr Apr 20, 2023

workingjubilee Apr 20, 2023

eeeebbbbrrrr Apr 20, 2023

workingjubilee Apr 20, 2023

workingjubilee Apr 20, 2023

eeeebbbbrrrr Apr 20, 2023

eeeebbbbrrrr Apr 20, 2023

eeeebbbbrrrr commented Apr 20, 2023

eeeebbbbrrrr commented Apr 20, 2023

workingjubilee commented Apr 20, 2023

workingjubilee commented Apr 20, 2023

workingjubilee commented Apr 20, 2023

workingjubilee commented Apr 20, 2023

eeeebbbbrrrr commented Apr 20, 2023

Use zero-copy Arrays in --release #1116

Use zero-copy Arrays in --release #1116

Conversation

workingjubilee commented Apr 20, 2023 • edited Loading

workingjubilee commented Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

eeeebbbbrrrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eeeebbbbrrrr commented Apr 20, 2023

eeeebbbbrrrr commented Apr 20, 2023

workingjubilee commented Apr 20, 2023

workingjubilee commented Apr 20, 2023

workingjubilee commented Apr 20, 2023

workingjubilee commented Apr 20, 2023

eeeebbbbrrrr commented Apr 20, 2023

Use zero-copy Arrays in `--release` #1116

Use zero-copy Arrays in `--release` #1116

workingjubilee commented Apr 20, 2023 •

edited

Loading

workingjubilee commented Apr 20, 2023 •

edited

Loading