Constexpr-ify inplace_vector #21

wusatosi · 2024-11-18T17:03:20Z

Make inplace_vector constexpr friendly by providing an T-based array storage alternative that avoids the needs of reinterpret_cast.

This storage alternative is only selected when std::is_trivial(T), as that's the requirement for enabling constexpr in the paper.

Adds constexpr based tests.

Related sections in paper:

constexpr support

Note: this revision brings back this section which was dropped in R8 because per LEWG discussion, LEWG thought that this whole type was implementable within constexpr for all types. The design intent of LEWG was to make it "as constexpr as possible."

The API of inplace_vector<T, Capacity> can be used in constexpr-contexts if is_trivially_copyable_v, is_default_constructible_v, and is_trivially_destructible are true. This proposal only supports using the constexpr methods in constant expressions if is_trivial_t is true.
The implementation cost of this is small. The prototye implementation specializes the storage to use a C array with value-initialized elements.
This negatively impacts the algorithmic complexity of inplace_vector constructors for these types from O(size) to O(capacity). When value-initialization takes place at run-time, this difference is significant.
Vectors with large capacity requirements are better served by vector instead.

All member functions should be constexpr.

Added a constexpr test that is not finished, I fear the code-fix resulted due to passing constexpr on all functions would result in too big of a PR. Currently [container.reqmts] related requirement is tested and fixed for int type.

Closes #17

include/beman/inplace_vector/inplace_vector.hpp

This reverts commit 431b0f1.

jbab

I basically agree with your changes. Most of my remarks (and some serious bugs) are in the code you didn't change (some even where GitHub doesn't allow me to put a comment).
Not sure how to handle this. Maybe it's good when your PR gets to the main line and then we can work on the rest.

include/beman/inplace_vector/inplace_vector.hpp

jbab · 2024-11-25T17:56:45Z

include/beman/inplace_vector/inplace_vector.hpp

+  T *begin() { return std::launder(reinterpret_cast<const T *>(elems)); }
+  const T *begin() const {
+    return std::launder(reinterpret_cast<const T *>(elems));
+  }


What is the reason you are using std::launder here?

This is to be consistent with previous implementation, I am bit confused myself too. I am not in a good place to make judgement call here, though I suspect it might be needed when T is const.

I don't think we should change this.

include/beman/inplace_vector/inplace_vector.hpp

jbab · 2024-11-25T18:30:35Z

include/beman/inplace_vector/inplace_vector.hpp

  internal_size_type size_{0};

  // [containers.sequences.inplace.vector.cons], construct/copy/destroy
  constexpr inplace_vector_destruct_base() = default;

-  inplace_vector_destruct_base(
+  constexpr inplace_vector_destruct_base(
      const inplace_vector_destruct_base
          &other) noexcept(std::is_nothrow_copy_constructible_v<T>)


Not your change, but to me this is unconditionally noexcept, since no T's are copied at all.

~~okay I will mark a TODO here. There's a lot of non-conformity in this implementation.~~

I think we should defer this to another PR, there's too much to be updated.

jbab · 2024-11-25T18:31:19Z

include/beman/inplace_vector/inplace_vector.hpp

      std::is_nothrow_copy_constructible_v<T> &&
      std::is_nothrow_copy_assignable_v<T>) {


Not your change, but to me this is unconditionally noexcept, since no T's are copied at all.

jbab · 2024-11-25T18:33:55Z

include/beman/inplace_vector/inplace_vector.hpp

      std::is_nothrow_move_constructible_v<T> &&
      std::is_nothrow_move_assignable_v<T>) {


Not your change, but to me this is unconditionally noexcept, since no T's are copied at all.

jbab · 2024-11-25T18:52:11Z

include/beman/inplace_vector/inplace_vector.hpp

  internal_size_type size_{0};

  // [containers.sequences.inplace.vector.cons], construct/copy/destroy
  constexpr inplace_vector_destruct_base() = default;

-  inplace_vector_destruct_base(
+  constexpr inplace_vector_destruct_base(
      const inplace_vector_destruct_base
          &other) noexcept(std::is_nothrow_copy_constructible_v<T>)
      : elems(), size_(other.size_) {}


Not your change, but I need to make some remark on all constructors in inplace_vector_destruct_base.
IMO, it would be O.K. to address this in a later PR.

They all value-initialise elems, i.e. the bytes in bytes_based_Storage become 0 and all elements in type_based_storage are value initialised. I tried to argue above why I don't like this, they should be initialised only when a vector element is really constructed in the place.

Besides (superfluously) initialising elems, they all do the same thing, they set size_ to some value. That is, they avoid doing what a copy c'tor, move c'tor etc. normally does and instead manipulate size_only. This is redundant. Instead, a single constructor taking a size would do. This could be called by the all the constructors in the deriving class. size_could probably be made private.

inplace_vector_destruct_base needs more work on trivially destructible requirement as well, needs to switch between a trivially destructible base and a non-trivial destructor against T.

Unfortunately intention in this repo is to support C++17, so we cannot use requires. This have to be done with template.

jbab · 2024-11-25T18:54:11Z

include/beman/inplace_vector/inplace_vector.hpp

      std::is_nothrow_copy_constructible_v<T>)
-      : inplace_vector_destruct_base<T, Capacity>(other.size) {
+      : inplace_vector_destruct_base<T, Capacity>(other.size_) {
    std::copy(other.begin(), other.end(), begin());


Not your change, but needs to be an uninitialised_copy, in particular for the byte based storage case.
It happens work in the type based storage case.

why is uninitialised_copy needed here?

jbab · 2024-11-25T18:55:18Z

include/beman/inplace_vector/inplace_vector.hpp

    std::copy(other.begin(), other.end(), begin());
  }
-  inplace_vector_base(inplace_vector_base &&other) noexcept(
+  constexpr inplace_vector_base(inplace_vector_base &&other) noexcept(
      Capacity == 0 || std::is_nothrow_move_constructible_v<T>)
      : inplace_vector_destruct_base<T, Capacity>(other.size_) {
    std::copy(other.begin(), other.end(), begin());


Not your change, but must be uninitalized_move, see above.

wusatosi · 2024-11-25T20:55:48Z

I basically agree with your changes. Most of my remarks (and some serious bugs) are in the code you didn't change (some even where GitHub doesn't allow me to put a comment). Not sure how to handle this. Maybe it's good when your PR gets to the main line and then we can work on the rest.

Thank you for review jbab. I agree that there's a lot of problems in this current implementation. That's why I am not attempting to setup a full test suite for constexpr. I think it will cause too big of a change.

This is just my thoughts and I haven't really discussed this in beman. But I am reaching out to david to see if we should just adopt the reference implementation in the paper (if the original author allows). As there's a lot of work to be done here and the reference implementation will be a better place to be worked off.

Edit: okay I figured this out myself... Here's the paper allowing default trivial initialization.

I don't get why inplace_vector have that note on the constexpr support. I will see if I can shoot the authors an email.

Previous confusion

Regarding storage, mostly due to my own limited understanding, I am not sure how constexpr plays with default-initialized memory, I thought all value must be initialized to a defined state to play nice with constexpr.

e.g. I thought func in the follow code is not constexpr because a is not initialized.

constexpr int func() {
    int a[5];
    a[0] = 1;
    return a[0];
}

Which is false because this passes godbolt...

However, in the paper, it mentions this wording:

Constexpr support
...
This negatively impacts the algorithmic complexity of inplace_vector constructors for these types from O(size) to O(capacity). When value-initialization takes place at run-time, this difference is significant.

Which suggest we need to value-initialize the T based array to make it play well with constexpr (use std::array storage<T, Capacity>{}). Which would suggest my understanding is correct, which makes me more confused.

I mean compiler seem to be fine with it, I just want to make sure this is intended per standard.

I tried to check with cppreference on constexpr, but I don't see how this is specified either way.

Edit:
The same godbolt code doesn't work on 17, but we aren't supporting constexpr for C++17 (at least for now) so we should be fine.

wusatosi · 2024-11-25T20:58:13Z

Btw, it is likely we will need to update this storage container to include size to ensure this fits an empty struct requirement when Capacity = 0. But again I want to keep this PR small so I will add this later.

wusatosi · 2024-11-25T20:59:00Z

@jbab If you don't mind, can you post an issue regarding questionable restraints surrounding constructors. I will deal with them in another PR.

Co-authored-by: Jan Babst <jan.babst@gmail.com>

include/beman/inplace_vector/inplace_vector.hpp

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

wusatosi · 2024-11-26T04:23:35Z

Hey @jbab , I've been given the green light to adopt the reference implementation from the paper.

I will be syncing that to a new branch and do a merge next.

Let's scrutinize the reference implementation first, if we have time we can come back and make current implementation great

wusatosi commented Nov 18, 2024

View reviewed changes

include/beman/inplace_vector/inplace_vector.hpp Outdated Show resolved Hide resolved

jbab reviewed Nov 19, 2024

View reviewed changes

include/beman/inplace_vector/inplace_vector.hpp Outdated Show resolved Hide resolved

include/beman/inplace_vector/inplace_vector.hpp Outdated Show resolved Hide resolved

include/beman/inplace_vector/inplace_vector.hpp Outdated Show resolved Hide resolved

wusatosi force-pushed the constexpr branch from c9f2d6e to 766ca64 Compare November 19, 2024 21:40

wusatosi added 6 commits November 21, 2024 12:14

initial implementation of constexpr

48d7243

minor format + documentation improv

ba31ac9

pre-commit

f8517de

test const 0 capacity variants

cc75d2a

disable constexpr test when C++ version < 20

8799d0b

use std::byte

cfbc617

wusatosi force-pushed the constexpr branch from 1785db4 to cfbc617 Compare November 21, 2024 17:14

add tests for [container.reqmts]

446426c

wusatosi marked this pull request as ready for review November 21, 2024 20:14

wusatosi requested review from camio, Hels15 and DeveloperPaul123 as code owners November 21, 2024 20:14

wusatosi added 3 commits November 21, 2024 15:52

slight refactoring

3ba7896

const variant is not needed

431b0f1

Revert "const variant is not needed"

12fe05e

This reverts commit 431b0f1.

wusatosi requested a review from jbab November 25, 2024 02:45

jbab reviewed Nov 25, 2024

View reviewed changes

wusatosi and others added 4 commits November 25, 2024 21:57

remove inplace_vector_array_type and add const tests

541ad68

fix const

6b75318

default initialize array

8dbc733

Update include/beman/inplace_vector/inplace_vector.hpp

5ff463a

Co-authored-by: Jan Babst <jan.babst@gmail.com>

github-actions bot reviewed Nov 26, 2024

View reviewed changes

include/beman/inplace_vector/inplace_vector.hpp Outdated Show resolved Hide resolved

wusatosi and others added 2 commits November 25, 2024 22:41

Update include/beman/inplace_vector/inplace_vector.hpp

3ec2600

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

fix ilformed constructor

64fdedc

wusatosi requested a review from jbab November 26, 2024 04:23

wusatosi closed this Nov 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Constexpr-ify inplace_vector #21

Constexpr-ify inplace_vector #21

wusatosi commented Nov 18, 2024 •

edited

Loading

jbab left a comment

jbab Nov 25, 2024

wusatosi Nov 25, 2024 •

edited

Loading

jbab Nov 25, 2024

wusatosi Nov 25, 2024 •

edited

Loading

jbab Nov 25, 2024

jbab Nov 25, 2024

jbab Nov 25, 2024

wusatosi Nov 26, 2024 •

edited

Loading

jbab Nov 25, 2024

wusatosi Nov 26, 2024

jbab Nov 25, 2024

wusatosi commented Nov 25, 2024 •

edited

Loading

wusatosi commented Nov 25, 2024

wusatosi commented Nov 25, 2024

wusatosi commented Nov 26, 2024 •

edited

Loading

		std::is_nothrow_copy_constructible_v<T> &&
		std::is_nothrow_copy_assignable_v<T>) {

		std::is_nothrow_move_constructible_v<T> &&
		std::is_nothrow_move_assignable_v<T>) {

Constexpr-ify inplace_vector #21

Constexpr-ify inplace_vector #21

Conversation

wusatosi commented Nov 18, 2024 • edited Loading

jbab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wusatosi Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wusatosi Nov 25, 2024 • edited Loading

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

wusatosi Nov 26, 2024 • edited Loading

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

wusatosi commented Nov 25, 2024 • edited Loading

wusatosi commented Nov 25, 2024

wusatosi commented Nov 25, 2024

wusatosi commented Nov 26, 2024 • edited Loading

wusatosi commented Nov 18, 2024 •

edited

Loading

wusatosi Nov 25, 2024 •

edited

Loading

wusatosi Nov 25, 2024 •

edited

Loading

wusatosi Nov 26, 2024 •

edited

Loading

wusatosi commented Nov 25, 2024 •

edited

Loading

wusatosi commented Nov 26, 2024 •

edited

Loading