RS NF4: support encoding/decoding using packets #215

lamphamsy · 2018-06-14T08:58:01Z

RS over gf::NF4 can encode/decode using packets now

slaperche-scality · 2018-07-27T12:15:29Z

src/gf_ring.h

@@ -379,7 +379,7 @@ void RingModN<T>::mul_coef_to_buf(T a, T* src, T* dest, size_t len) const
    DoubleSizeVal<T> coef = DoubleSizeVal<T>(a);
    for (i = 0; i < len; i++) {
        // perform multiplication
-        dest[i] = T((coef * src[i]) % this->_card);
+        dest[i] = mul(coef, src[i]);


Instead of having a method mul, could we overload the * operator? That would make the code more readable.

This kind of changes would be in a separated PR.

Yeah, you don't need to handle it here, I'll do it in #208

slaperche-scality · 2018-07-27T12:15:34Z

src/gf_ring.h

@@ -404,7 +404,7 @@ void RingModN<T>::add_two_bufs(T* src, T* dest, size_t len) const
    size_t i;
    for (i = 0; i < len; i++) {
        // perform addition
-        dest[i] = (src[i] + dest[i]) % this->_card;
+        dest[i] = add(src[i], dest[i]);


Instead of having a method add, could we overload the + operator? That would make the code more readable.

slaperche-scality · 2018-07-27T12:15:42Z

src/gf_ring.h

-            result = this->_card - (bufb[i] - bufa[i]);
-        }
-        res[i] = result;
+        res[i] = sub(bufa[i], bufb[i]);


Instead of having a method sub, could we overload the - operator? That would make the code more readable.

If these operations are the one from RingModN, you could replace:

virtual T neg(T a) const; virtual T add(T a, T b) const; virtual T sub(T a, T b) const; virtual T mul(T a, T b) const; virtual T div(T a, T b) const;

by

virtual T operator-() const; virtual T operator+(T lhs, T rhs) const; virtual T operator-(T lhs, T rhs) const; virtual T operator*(T lhs, T rhs) const; virtual T operator/(T lhs, T rhs) const;

(replace T by const T& if the type are not limited to built-in integers, to avoid useless copy)

slaperche-scality · 2018-07-27T12:29:11Z

src/simd_nf4.h

@@ -128,7 +128,7 @@ inline void unpack(__uint128_t a, GroupedValues<__uint128_t>& b, int n)
    _mm_store_si128((m128i*)&values, val);

    b.flag = flag;
-    b.values = values;
+    b.values = values; // NOLINT


Why?

You should explain what are you disabling here (with a comment/in the commit msg), or at least disable a single lint instead of all of them.

slaperche-scality · 2018-07-27T12:49:22Z

src/simd_nf4.h

+    ai[7] = _mm_extract_epi16(_a, 7);
+
+    flag = ai[1];
+    flag += (ai[3] > 0) ? 2 : 0;


You could avoid the branches (but I expect the compiler to be smart enough to remove them anyway) by writting the code as flag = ai[1] | (!!ai[3] << 1u) | (!!ai[5] << 2u) | (!!ai[7] << 3u);

You forgot this one (but did it for inline GroupedValues<__uint128_t> unpack(__uint128_t a, int n)

slaperche-scality · 2018-07-27T12:58:04Z

test/ec_driver.cpp

@@ -360,7 +360,8 @@ template <typename T>
 void run_fec_rs_nf4(int word_size, int n_data, int n_parities, int rflag)
 {
    quad::fec::RsNf4<T>* fec;
-    fec = new quad::fec::RsNf4<T>(word_size, n_data, n_parities);
+    size_t pkt_size = 1024;
+    fec = new quad::fec::RsNf4<T>(word_size, n_data, n_parities, pkt_size);


Use a std::unique_ptr

This kind of enhancement will be in a separated PR

slaperche-scality · 2018-07-27T12:58:25Z

src/simd_nf4.h

@@ -49,7 +49,7 @@ static inline aint128 m128i_to_uint128(m128i v)
 }
 #endif // #ifdef QUADIRON_USE_AVX2

-inline aint128 expand16(aint16* arr, int n)
+inline aint128 expand16(uint16_t* arr, int n)


why this change?

It should be explained in the commit message.

slaperche-scality · 2018-07-27T12:59:26Z

benchmark/benchmark.cpp

@@ -171,7 +171,7 @@ int Benchmark<T>::init()
        fec = new quad::fec::RsGfpFft<T>(word_size, k, m);
        break;
    case EC_TYPE_RS_NF4:
-        fec = new quad::fec::RsNf4<T>(word_size, k, m);
+        fec = new quad::fec::RsNf4<T>(word_size, k, m, pkt_size);


You should probably use std::unique_ptr instead of raw new in order to have auto-delete

This kind of enhancement will be in a separated PR

slaperche-scality · 2018-07-27T12:59:49Z

src/simd_nf4.h

@@ -102,6 +102,35 @@ inline GroupedValues<__uint128_t> unpack(__uint128_t a, int n)
    return b;
 }

+inline void unpack(__uint128_t a, GroupedValues<__uint128_t>& b, int n)


See my comments on inline GroupedValues<__uint128_t> unpack(__uint128_t a, int n)

slaperche-scality · 2018-07-27T13:17:15Z

src/fec_rs_nf4.h

+            std::vector<size_t> packed_symbs;
+            // pack marked symbols
+            for (auto const& data : props[frag_id].get_map()) {
+                off_t loc_offset = data.first.get_offset();


lamphamsy · 2018-08-07T10:53:09Z

@slaperche-scality : it's ready for next reviews. Thanks :)

slaperche-scality

Almost good, next iteration should be good for merge 🙂

slaperche-scality · 2018-08-13T09:09:52Z

src/simd_nf4.h

+    ai[7] = _mm_extract_epi16(_a, 7);
+
+    flag = ai[1];
+    flag += (ai[3] > 0) ? 2 : 0;


You forgot this one (but did it for inline GroupedValues<__uint128_t> unpack(__uint128_t a, int n)

Use virtualized basic operations instead of implementing directly for prime fields

It removes align attributes that are not necessary.

lamphamsy force-pushed the ft/nf4_packet branch from 9997c1c to f75c7d9 Compare June 22, 2018 10:43

vrancurel approved these changes Jun 24, 2018

View reviewed changes

lamphamsy force-pushed the ft/decode_buffers branch from 20e26a4 to f362e7b Compare July 27, 2018 05:55

lamphamsy force-pushed the ft/nf4_packet branch from f75c7d9 to 2dbbcd0 Compare July 27, 2018 10:20

slaperche-scality suggested changes Jul 27, 2018

View reviewed changes

lamphamsy force-pushed the ft/nf4_packet branch 2 times, most recently from 6879e22 to 4091482 Compare August 7, 2018 10:25

lamphamsy changed the base branch from ft/decode_buffers to master August 7, 2018 10:26

lamphamsy force-pushed the ft/nf4_packet branch from 4091482 to 63e74e8 Compare August 7, 2018 10:32

lamphamsy force-pushed the ft/nf4_packet branch from 63e74e8 to 6239754 Compare August 7, 2018 11:49

slaperche-scality suggested changes Aug 13, 2018

View reviewed changes

lamphamsy force-pushed the ft/nf4_packet branch from 6239754 to 455fa8c Compare August 17, 2018 10:13

lamphamsy added 12 commits August 23, 2018 13:18

GF Ring: use virtualized basic operations

9fd7022

Use virtualized basic operations instead of implementing directly for prime fields

Vec buffer: fix output for dump

7d63a68

FEC NF4: encoding and decoding using packets

db0ccfc

FEC NF4: create FFT supporting packets

16b1ad0

EC driver: RS-NF4 using packets

3a762af

SIMD NF4: remove align attribute

e6c46f8

It removes align attributes that are not necessary.

Benchmark: allow RS-NF4 using packets

0030376

SIMD NF4: enhance unpack

2744b1c

GF NF4: add other unpack function working on given GroupedValues

9573dfa

FEC NF4: use new unpack

fe2c300

SIMD NF4: add nolint

d257a9c

Address comments

d78fe68

lamphamsy force-pushed the ft/nf4_packet branch from 455fa8c to d78fe68 Compare August 23, 2018 11:19

slaperche-scality approved these changes Aug 24, 2018

View reviewed changes

lamphamsy merged commit 73766c8 into master Aug 24, 2018

lamphamsy deleted the ft/nf4_packet branch August 31, 2018 08:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RS NF4: support encoding/decoding using packets #215

RS NF4: support encoding/decoding using packets #215

lamphamsy commented Jun 14, 2018

slaperche-scality Jul 27, 2018

lamphamsy Aug 7, 2018

slaperche-scality Aug 13, 2018

slaperche-scality Jul 27, 2018

slaperche-scality Jul 27, 2018

slaperche-scality Jul 27, 2018

slaperche-scality Jul 27, 2018

slaperche-scality Aug 13, 2018

slaperche-scality Jul 27, 2018

lamphamsy Aug 7, 2018

slaperche-scality Jul 27, 2018

slaperche-scality Jul 27, 2018

lamphamsy Aug 7, 2018

slaperche-scality Jul 27, 2018

slaperche-scality Jul 27, 2018

lamphamsy commented Aug 7, 2018

slaperche-scality left a comment

slaperche-scality Aug 13, 2018

RS NF4: support encoding/decoding using packets #215

RS NF4: support encoding/decoding using packets #215

Conversation

lamphamsy commented Jun 14, 2018

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

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

lamphamsy commented Aug 7, 2018

slaperche-scality left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment