Introduce Span type and use it instead of FLATDATA

sipa commented at 7:43 pm on April 4, 2018: member

Introduce a new data type Span, which is an encapsulated pointer + size (like C++20’s std::span or LevelDB’s Slice), and represents a view to a sequence of objects laid out continuously in memory.

The immediate use case is replacing the remaining FLATDATA invocations. Instead of those, we support serializing/deserializing unsigned char Spans (treating them as arrays).

A longer term goal for Spans is making the script execution operate on them rather than on CScript itself. This will allow separate storage mechanisms for scripts.

ryanofsky commented at 8:45 pm on April 4, 2018: member

Slice type seems similar to upcoming std::span. It may be a good idea to call this class Span and consciously keep the implementation compatible so it could be swapped out in the future (it does seem compatible already).

sipa commented at 8:53 pm on April 4, 2018: member

Cool, I’ll rename it and add a comment. I wasn’t aware of std::span.

Also, std::span seems to have a constructor that behaves like the MakeSlice function I have here. I don’t think it’s possible to implement that as a constructor in c++11 yet.

practicalswift commented at 9:09 pm on April 4, 2018: contributor

Concept ACK. Very nice!

Does this need a test?

in src/slice.h:37 in 347405bfc9 outdated

32+ *
33+ * This correctly deals with constness: if either the element type is const
34+ * or a reference to a const container is passed, the result will be a const
35+ * Slice. */
36+template<typename V>
37+Slice<typename std::remove_pointer<decltype(std::declval<V>().data())>::type> MakeSlice(V& v) { return Slice<typename std::remove_pointer<decltype(std::declval<V>().data())>::type>(v.data(), v.size()); }

theuni commented at 9:40 pm on April 4, 2018:

This could use an overload for the std::is_trivially_copyable trait that creates a slice for Slice<uint8_t>(&v, sizeof(v)):

Objects of trivially-copyable types are the only C++ objects that may be safely copied with std::memcpy or serialized to/from binary files with std::ofstream::write()/std::ifstream::read(). In general, a trivially copyable type is any type for which the underlying bytes can be copied to an array of char or unsigned char and into a new object of the same type, and the resulting object would have the same value as the original.

That would eliminate the need for the hack in netaddress.h.

theuni commented at 9:41 pm on April 4, 2018: member

Concept ACK, way overdue.

sipa commented at 9:46 pm on April 4, 2018: member

@theuni My idea of removing the hack in netaddress is introducing a BigInteger wrapper (see #10785).

sipa force-pushed on Apr 4, 2018

sipa commented at 11:23 pm on April 4, 2018: member

A few small changes:

Renamed to Span
Reduced the number of methods (it’s now so trivial that I don’t see what could be tested about it, @practicalswift).
Added comments to explain similarity with C++20’s std::span (thanks @ryanofsky).
Added TODO on the netaddress port serialization hack (pointed out by @theuni).

sipa force-pushed on Apr 4, 2018

sipa renamed this:
~~Introduce Slice type and use it instead of FLATDATA~~
Introduce Span type and use it instead of FLATDATA
on Apr 4, 2018

sipa force-pushed on Apr 5, 2018

in src/span.h:38 in ce141afc5a outdated

33+ * or its element type is const, the resulting span will have a const element type.
34+ *
35+ * std::span will have a constructor that implements this functionality directly.
36+ */
37+template<typename V>
38+constexpr Span<typename std::remove_pointer<decltype(std::declval<V>().data())>::type> MakeSpan(V& v) { return Span<typename std::remove_pointer<decltype(std::declval<V>().data())>::type>(v.data(), v.size()); }

Empact commented at 0:57 am on April 5, 2018:

clang-format will flatten this out and make some other whitespace changes

laanwj commented at 6:49 am on April 5, 2018: member

utACK https://github.com/bitcoin/bitcoin/pull/12886/commits/ce141afc5a4a322b309840789e984b0a79c603fb

A longer term goal for Spans is making the script execution operate on them rather than on CScript itself. This will allow separate storage mechanisms for scripts.

Finally :)

@theuni My idea of removing the hack in netaddress is introducing a BigInteger wrapper (see #10785).

Good idea, though the name BigInteger put me on the wrong foot initially. Better to spell it out as BigEndianInteger probably.

theuni commented at 2:47 pm on April 5, 2018: member

@sipa Sounds good.

My only complaint remaining was that member functions weren’t constexpr, but that has apparently been fixed in the meantime. utACK ce141afc5a4a322b309840789e984b0a79c603fb

sipa commented at 2:50 pm on April 5, 2018: member

@laanwj Oops, typo. The wrapper is called BigEndian in #10785.

theuni commented at 2:50 pm on April 5, 2018: member

Quick follow-up, we’ll want to add data() to any containers that don’t have it already. base_blob (uint256) for example. No need to do it in this PR though.

Add Slice: a (pointer, size) array view that acts like a container 833bc08583

Support serializing Span<unsigned char> and use that instead of FLATDATA 9272d70536

in src/span.h:25 in ce141afc5a outdated

20+
21+public:
22+    constexpr Span() noexcept : m_data(nullptr), m_size(0) {}
23+    constexpr Span(C* data, std::ptrdiff_t size) noexcept : m_data(data), m_size(size) {}
24+
25+    constexpr C* data() noexcept { return m_data; }

theuni commented at 2:54 pm on April 5, 2018:

Tiny nit: I think these should be const too, as it’s not implied by constexpr in c++14.

sipa commented at 3:21 pm on April 5, 2018:

Fixed.

sipa force-pushed on Apr 5, 2018

laanwj added the label Refactoring on Apr 5, 2018

in src/span.h:40 in 9272d70536

35+ * std::span will have a constructor that implements this functionality directly.
36+ */
37+template<typename V>
38+constexpr Span<typename std::remove_pointer<decltype(std::declval<V>().data())>::type> MakeSpan(V& v) { return Span<typename std::remove_pointer<decltype(std::declval<V>().data())>::type>(v.data(), v.size()); }
39+
40+#endif

promag commented at 1:23 pm on April 8, 2018:

0#endif // BITCOIN_SPAN_H

promag commented at 1:26 pm on April 8, 2018: member

utACK.

promag commented at 1:28 pm on April 8, 2018: member

Rename Slice to Span in first commit message.

in src/netaddress.h:174 in 9272d70536

167@@ -167,10 +168,13 @@ class CService : public CNetAddr
168         template <typename Stream, typename Operation>
169         inline void SerializationOp(Stream& s, Operation ser_action) {
170             READWRITE(ip);
171+
172+            // TODO: introduce native support for BE serialization in serialize.h
173             unsigned short portN = htons(port);
174-            READWRITE(FLATDATA(portN));
175-            if (ser_action.ForRead())
176+            READWRITE(Span<unsigned char>((unsigned char*)&portN, 2));

jonasschnelli commented at 2:16 pm on April 8, 2018:

not sure, but maybe use sizeof(uint16_t) instead of 2 to increase code readability? Or use uint16_t as htons container (instead of unsigned short) and sizeof(portN)?

laanwj commented at 3:56 pm on April 8, 2018:

Yep, but this code is going away anyhow see

@theuni My idea of removing the hack in netaddress is introducing a Big~~Integer~~Endian wrapper (see #10785).

jonasschnelli commented at 2:17 pm on April 8, 2018: contributor

utACK 9272d70536287d4ff9aa1ee41a401465c0e8194a

laanwj merged this on Apr 8, 2018

laanwj closed this on Apr 8, 2018

laanwj referenced this in commit 97785863e2 on Apr 8, 2018

UdjinM6 referenced this in commit 4bb1132438 on Dec 17, 2020

furszy referenced this in commit 5c93f159bc on Jul 5, 2021

random-zebra referenced this in commit b4751e10ce on Aug 11, 2021

DrahtBot locked this on Sep 8, 2021

Introduce Span type and use it instead of FLATDATA #12886