posix: replace ctree with direct offset mapping tree

[igchor]

This change is 

[codecov[bot]]

Codecov Report

Merging #397 into master will increase coverage by 0.66%. The diff coverage is 85.25%.

@@            Coverage Diff             @@
##           master     #397      +/-   ##
==========================================
+ Coverage   75.95%   76.61%   +0.66%     
==========================================
  Files          70       71       +1     
  Lines        8815     8768      -47     
  Branches     1781     1778       -3     
==========================================
+ Hits         6695     6718      +23     
+ Misses       1628     1553      -75     
- Partials      492      497       +5

Flag	Coverage Δ
#ltp_tests	45.34% <81.93%> (+0.59%)	:arrow_up:
#sqlite_tests	45.51% <80%> (+0.63%)	:arrow_up:
#tests_antool	15.13% <0%> (+0.08%)	:arrow_up:
#tests_posix_multi_threaded	25.5% <41.66%> (-0.05%)	:arrow_down:
#tests_posix_single_threaded	54.12% <83.97%> (+0.62%)	:arrow_up:
#tests_preload	44.98% <57.05%> (+0.56%)	:arrow_up:

Impacted Files	Coverage Δ
src/libpmemfile-posix/inode.h	100% <ø> (ø)	:arrow_up:
src/libpmemfile-posix/inode.c	89.07% <100%> (ø)	:arrow_up:
src/libpmemfile-posix/block_array.c	96.74% <50%> (ø)	:arrow_up:
src/libpmemfile-posix/offset_mapping.c	85% <85%> (ø)
src/libpmemfile-posix/data.c	94.09% <90.9%> (-0.03%)	:arrow_down:
src/libpmemfile-posix/alloc.c	48.64% <0%> (-5.41%)	:arrow_down:
src/libpmemfile-posix/out.h	0% <0%> (ø)
src/libpmemfile/path_resolve.c	89.28% <0%> (+0.59%)	:arrow_up:
src/libpmemfile-posix/fcntl.c	49.38% <0%> (+2.46%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7f181b5...f7dc018. Read the comment docs.

[krzycz]

Reviewed 7 of 9 files at r1. Review status: 7 of 9 files reviewed at latest revision, 1 unresolved discussion.

---

src/libpmemfile-posix/offset_mapping.c, line 32 at r1 (raw file):

 * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 */
#include <stdio.h>

any brief?

---

Comments from Reviewable

[GBuella]

Reviewed 8 of 9 files at r1. Review status: 8 of 9 files reviewed at latest revision, 8 unresolved discussions.

---

src/libpmemfile-posix/CMakeLists.txt, line 94 at r1 (raw file):

  out.h
  pool.h
  offset_mapping.h

Almost alphabetical order

---

src/libpmemfile-posix/data.c, line 67 at r1 (raw file):

find_last_block(const struct pmemfile_vinode *vinode)
{
  return block_find_closest(vinode->blocks, UINT64_MAX);

Looks much better!

---

src/libpmemfile-posix/offset_mapping.c, line 41 at r1 (raw file):

#define N_CHILDREN_POW 4
#define N_CHILDREN (1 << N_CHILDREN_POW)

So, this will be a tree, with a branching factor of 16, right? Each level has an array of 16 entries, each of which can internal, or leaf, right?

---

src/libpmemfile-posix/offset_mapping.c, line 68 at r1 (raw file):

 */
COMPILE_ERROR_ON(RANGE(ARRAY_SIZE(range) - 1) == 0);
COMPILE_ERROR_ON(RANGE(ARRAY_SIZE(range)) != 0);

Doesn't some compiler complain about that? Perhaps that warning is not enabled? I mean, we know that is what we really want here, but poor compiler doesn't understand that, I would guess.

---

src/libpmemfile-posix/offset_mapping.c, line 76 at r1 (raw file):

offset_map_new(PMEMfilepool *pfp)
{
  struct offset_map *m = pf_calloc(1, sizeof(struct offset_map));

pf_calloc(1, sizeof(*m));

---

src/libpmemfile-posix/offset_mapping.c, line 160 at r1 (raw file):

      children = entry->child;
      entry = &children[offset / range[level]];
      offset %= range[level];

BTW, we could just do the shift in this loop, instead of adding a lookup table. range >>= N_CHILDREN_POW

Generally, this could achieve the same thing as that lookup table:

static uint64_t range(int n)
{
     return UINT64_C(1) << (n * N_CHILDREN_POW);
}

Unless that lookup table is going to refer to non-constant branching factors.

---

src/libpmemfile-posix/offset_mapping.h, line 56 at r1 (raw file):

   */

  bool internal;

In a future version this one bit of information could be merged into that pointers least significant bit (using uintptr_t instead, or a two uintptr_t bitfields with sizes 63 and 1). Normally I don't like doing such ugly things, but in this case it can mean doubling the branching factor using the amounts allocated, and potentially making the tree less tall.

---

Comments from Reviewable

[igchor]

Review status: 6 of 9 files reviewed at latest revision, 8 unresolved discussions.

---

src/libpmemfile-posix/CMakeLists.txt, line 94 at r1 (raw file):

Previously, GBuella (Gabor Buella) wrote…

Almost alphabetical order

Done.

---

src/libpmemfile-posix/offset_mapping.c, line 32 at r1 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

any brief?

Done.

---

src/libpmemfile-posix/offset_mapping.c, line 41 at r1 (raw file):

Previously, GBuella (Gabor Buella) wrote…

So, this will be a tree, with a branching factor of 16, right? Each level has an array of 16 entries, each of which can internal, or leaf, right?

That's right.

---

src/libpmemfile-posix/offset_mapping.c, line 68 at r1 (raw file):

Previously, GBuella (Gabor Buella) wrote…

Doesn't some compiler complain about that? Perhaps that warning is not enabled? I mean, we know that is what we really want here, but poor compiler doesn't understand that, I would guess.

What warning would that be? I don't really have any other idea how to check whether last entry is max possible value.

---

src/libpmemfile-posix/offset_mapping.c, line 76 at r1 (raw file):

Previously, GBuella (Gabor Buella) wrote…

pf_calloc(1, sizeof(*m));

Done.

---

src/libpmemfile-posix/offset_mapping.c, line 160 at r1 (raw file):

Previously, GBuella (Gabor Buella) wrote…

BTW, we could just do the shift in this loop, instead of adding a lookup table. `range >>= N_CHILDREN_POW` Generally, this could achieve the same thing as that lookup table: ``` static uint64_t range(int n) { return UINT64_C(1) << (n * N_CHILDREN_POW); } ``` Unless that lookup table is going to refer to non-constant branching factors.

Well, actually this lookup table is there for performance reasons, I tested it with similar function for calculating range and there was some difference. On this plot: http://10.91.71.254:8080/view/Performance/job/pmemfile_fork_performance_sqlite/plot/

build 24 was with lookup table and 25 with calculating range as you suggested.

---

Comments from Reviewable

[GBuella]

Reviewed 2 of 3 files at r2. Review status: 8 of 9 files reviewed at latest revision, 5 unresolved discussions.

---

src/libpmemfile-posix/offset_mapping.c, line 68 at r1 (raw file):

Previously, igchor wrote…

What warning would that be? I don't really have any other idea how to check whether last entry is max possible value.

I was thinking about this one:

$ cat shifting.c
unsigned long long x(unsigned long long arg)
{
    return arg << 65;
}

$ cc shifting.c -c 
shifting.c: In function ‘x’:
shifting.c:4:13: warning: left shift count >= width of type [-Wshift-count-overflow]
  return arg << 65;
             ^~

And I didn't even explicitly enable any warnings... Probably something silences it in the COMPILE_ERROR_ON macro, I'm not sure.

---

src/libpmemfile-posix/offset_mapping.c, line 160 at r1 (raw file):

Previously, igchor wrote…

Well, actually this lookup table is there for performance reasons, I tested it with similar function for calculating range and there was some difference. On this plot: http://10.91.71.254:8080/view/Performance/job/pmemfile_fork_performance_sqlite/plot/ build 24 was with lookup table and 25 with calculating range as you suggested.

wow, that is hard to understand why, but if you are sure

---

Comments from Reviewable

[GBuella]

Review status: 8 of 9 files reviewed at latest revision, 5 unresolved discussions.

---

src/libpmemfile-posix/offset_mapping.c, line 160 at r1 (raw file):

Previously, GBuella (Gabor Buella) wrote…

wow, that is hard to understand why, but if you are sure

I mean, compare:

• calculate a memory address in a lookup table, and load from the lookup table
• do a shift

---

Comments from Reviewable

[GBuella]

Review status: 8 of 9 files reviewed at latest revision, 5 unresolved discussions.

---

src/libpmemfile-posix/offset_mapping.c, line 160 at r1 (raw file):

Previously, GBuella (Gabor Buella) wrote…

I mean, compare: * calculate a memory address in a lookup table, and load from the lookup table * do a shift

Well, I looked at it, both compile to two instructions in the general case, and both can compile to a single instruction in this loop

---

Comments from Reviewable

[igchor]

Review status: 8 of 9 files reviewed at latest revision, 5 unresolved discussions.

---

src/libpmemfile-posix/offset_mapping.c, line 160 at r1 (raw file):

Previously, GBuella (Gabor Buella) wrote…

Well, I looked at it, both compile to two instructions in the general case, and both can compile to a single instruction in this loop

Well, actually I tested it not using (range >>= N_CHILDREN) but function similar to your's: static uint64_t range(int n) { return MIN_BLOCK_SIZE << (N_CHILDREN * (n - 1)); }

But probably you're right, I should just do: range >>= N_CHILDREN_POW. I dind't use that previously because I wasn't sure if branching factor should be const for each level.

---

Comments from Reviewable

[igchor]

Review status: 7 of 9 files reviewed at latest revision, 5 unresolved discussions.

---

src/libpmemfile-posix/offset_mapping.c, line 68 at r1 (raw file):

Previously, GBuella (Gabor Buella) wrote…

I was thinking about this one: ``` $ cat shifting.c unsigned long long x(unsigned long long arg) { return arg << 65; } $ cc shifting.c -c shifting.c: In function ‘x’: shifting.c:4:13: warning: left shift count >= width of type [-Wshift-count-overflow] return arg << 65; ^~ ``` And I didn't even explicitly enable any warnings... Probably something silences it in the COMPILE_ERROR_ON macro, I'm not sure.

Okay, it's not needed anymore.

---

src/libpmemfile-posix/offset_mapping.c, line 160 at r1 (raw file):

Previously, igchor wrote…

Well, actually I tested it not using (range >>= N_CHILDREN) but function similar to your's: static uint64_t range(int n) { return MIN_BLOCK_SIZE << (N_CHILDREN * (n - 1)); } But probably you're right, I should just do: range >>= N_CHILDREN_POW. I dind't use that previously because I wasn't sure if branching factor should be const for each level.

Done.

---

Comments from Reviewable

[GBuella]

Reviewed 1 of 2 files at r3. Review status: 8 of 9 files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[marcinslusarz]

Reviewed 5 of 9 files at r1, 1 of 3 files at r2. Review status: 8 of 9 files reviewed at latest revision, 15 unresolved discussions.

---

src/libpmemfile-posix/block_array.c, line 477 at r3 (raw file):

      remove_block(vinode->blocks, moving_block);
      relocate_block(pfp, block, moving_block);
      insert_block(vinode->blocks, block);

insert_block can fail, so I think error handling should stay

---

src/libpmemfile-posix/data.c, line 37 at r3 (raw file):

#include "data.h"
#include "out.h"
#include "offset_mapping.h"

f < u

---

src/libpmemfile-posix/inode.h, line 42 at r3 (raw file):

#include "layout.h"
#include "os_thread.h"
#include "offset_mapping.h"

f < s

---

src/libpmemfile-posix/offset_mapping.c, line 169 at r3 (raw file):

  if (entry->child != NULL) {
      return entry->child;
  } else {

s/} else {//

---

src/libpmemfile-posix/offset_mapping.c, line 218 at r3 (raw file):

 * frees memory used by 'child' if  all child entries are NULL
 */

remove empty line please

---

src/libpmemfile-posix/offset_mapping.c, line 307 at r3 (raw file):

  }

too many empty lines

---

src/libpmemfile-posix/offset_mapping.c, line 327 at r3 (raw file):

   * cleans up offset_map tree
   * if at the top level only first entry is internal and not null
   * it's children can be transfered one level up and height of

its, transferred

---

src/libpmemfile-posix/offset_mapping.c, line 328 at r3 (raw file):

   * if at the top level only first entry is internal and not null
   * it's children can be transfered one level up and height of
   * the tree can be decresed

decreased

---

src/libpmemfile-posix/offset_mapping.c, line 340 at r3 (raw file):

              return 0;

          /* check if all entries excpet first are NULL */

except

---

src/libpmemfile-posix/offset_mapping.h, line 47 at r3 (raw file):

   * child points to pmemfile_block_desc when internal == false
   * or to offset_map_entry array otherwise
   */

Please move this comment before child field. I'm not sure it's worth it, but you could use union for the child field...

---

src/libpmemfile-posix/offset_mapping.h, line 58 at r3 (raw file):

  PMEMfilepool *pfp;

  uint64_t range;

comment before this field please

---

src/libpmemfile-posix/offset_mapping.h, line 63 at r3 (raw file):

struct pmemfile_block_desc;

too many empty lines

---

Comments from Reviewable

[krzycz]

Reviewed 1 of 3 files at r2, 2 of 2 files at r3. Review status: all files reviewed at latest revision, 14 unresolved discussions.

---

Comments from Reviewable

[igchor]

Review status: 4 of 9 files reviewed at latest revision, 14 unresolved discussions.

---

src/libpmemfile-posix/block_array.c, line 477 at r3 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

insert_block can fail, so I think error handling should stay

Done.

---

src/libpmemfile-posix/data.c, line 37 at r3 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

f < u

Done.

---

src/libpmemfile-posix/inode.h, line 42 at r3 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

f < s

Done.

---

src/libpmemfile-posix/offset_mapping.c, line 169 at r3 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

s/} else {//

I'm not sure what you mean

---

src/libpmemfile-posix/offset_mapping.c, line 218 at r3 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

remove empty line please

Done.

---

src/libpmemfile-posix/offset_mapping.c, line 307 at r3 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

too many empty lines

Done.

---

src/libpmemfile-posix/offset_mapping.c, line 327 at r3 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

its, transferred

Done.

---

src/libpmemfile-posix/offset_mapping.c, line 328 at r3 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

decreased

Done.

---

src/libpmemfile-posix/offset_mapping.c, line 340 at r3 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

except

Done.

---

src/libpmemfile-posix/offset_mapping.h, line 47 at r3 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

Please move this comment before child field. I'm not sure it's worth it, but you could use union for the child field...

Done. I was also thinking about union but whenever we access child we also check/set 'internal' value so it's always clear whether child points to block or next level entries. However, if you think that adding union would make code more readable, I can do that.

---

src/libpmemfile-posix/offset_mapping.h, line 58 at r3 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

comment before this field please

Done.

---

src/libpmemfile-posix/offset_mapping.h, line 63 at r3 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

too many empty lines

Done.

---

Comments from Reviewable

[krzycz]

Reviewed 5 of 5 files at r4. Review status: all files reviewed at latest revision, 14 unresolved discussions.

---

Comments from Reviewable

[marcinslusarz]

Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

src/libpmemfile-posix/offset_mapping.c, line 169 at r3 (raw file):

Previously, igchor wrote…

I'm not sure what you mean

It's a sed expression. When one code path of "if" ends with "return" there's no point in "else" and indentation in the other path.

---

src/libpmemfile-posix/offset_mapping.h, line 47 at r3 (raw file):

Previously, igchor wrote…

Done. I was also thinking about union but whenever we access child we also check/set 'internal' value so it's always clear whether child points to block or next level entries. However, if you think that adding union would make code more readable, I can do that.

I was thinking about type safety and documenting of possible types. void* can be anything.

---

src/libpmemfile-posix/offset_mapping.h, line 58 at r3 (raw file):

Previously, igchor wrote…

Done.

"range" is usually 2 values (start and length, or start and end), so please document what is stored in "range" or rename it to something clearer

---

Comments from Reviewable

[igchor]

Review status: 7 of 9 files reviewed at latest revision, 3 unresolved discussions.

---

src/libpmemfile-posix/offset_mapping.c, line 169 at r3 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

It's a sed expression. When one code path of "if" ends with "return" there's no point in "else" and indentation in the other path.

Done.

---

src/libpmemfile-posix/offset_mapping.h, line 47 at r3 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

I was thinking about type safety and documenting of possible types. void* can be anything.

Done. However, I'm not sure if 'data' is a good name for an union.

---

src/libpmemfile-posix/offset_mapping.h, line 58 at r3 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

"range" is usually 2 values (start and length, or start and end), so please document what is stored in "range" or rename it to something clearer

Ok, you're right, probably range_length will be a better name.

---

Comments from Reviewable

[GBuella]

Review status: 7 of 9 files reviewed at latest revision, 4 unresolved discussions.

---

src/libpmemfile-posix/offset_mapping.c, line 162 at r5 (raw file):

      range >>= N_CHILDREN_POW;
      children = entry->data.children;
      entry = &children[offset / range];

For the curious: I realized the compiler can't do strength reduction on the division here, since divisor comes form a variable -> doesn't know it is a power of two. I compiled this, and there is a `div` instruction in the middle of this loop. I managed to get rid of the division by using this uglier form: ``` int bit_index = __builtin_ctzll(range); /* cuz I know it has a single bit set, but the compiler doesn't know */ while (entry->internal) { children = entry->data.children; bit_index -= N_CHILDREN_POW; entry = children + ((offset >> bit_index) & ((1 << N_CHILDREN_POW) - 1)); } ```

---

Comments from Reviewable

[sarahjelinek]

Reviewed 4 of 9 files at r1, 1 of 3 files at r2, 3 of 5 files at r4. Review status: 7 of 9 files reviewed at latest revision, 5 unresolved discussions.

---

src/libpmemfile-posix/offset_mapping.c, line 40 at r5 (raw file):

 * one block at this range, offset_map_entry holds a pointer to an array of
 * next level offset_map_entries (with smaller range) - size of this array
 * is const and equal to N_CHILDREN defiend below

s/defiend/defined

---

Comments from Reviewable

[GBuella]

Reviewed 1 of 5 files at r4, 2 of 2 files at r5. Review status: all files reviewed at latest revision, 9 unresolved discussions.

---

src/libpmemfile-posix/block_array.c, line 478 at r5 (raw file):

      relocate_block(pfp, block, moving_block);
      if (insert_block(vinode->blocks, block))
          pmemfile_tx_abort(errno);

If inserting a block, then aborting the transaction?

---

src/libpmemfile-posix/offset_mapping.c, line 60 at r5 (raw file):

 * ---------------------------------------------------------------------------
 *                     ^(2)            ^(2)    ...       ^(2)
 * 16 entries will be updated (blocks covering offsets 240k - 496k)

I see this business of representing ranges with ranges of different overlapping ranges (sorry if I don't make any sense) is pretty complicated to implement, are there separate unit tests just for the tree? It might be useful. Is it correct with silly corner cases?

---

src/libpmemfile-posix/offset_mapping.c, line 180 at r5 (raw file):

  struct offset_map_entry *e;

  /* look for block in entries with lower offset */

Another little diagram here would be cool

---

src/libpmemfile-posix/offset_mapping.c, line 195 at r5 (raw file):

  /*
   * look for block in entries with higher offset,
   * if found, then return previous block

And a little diagram here

---

Comments from Reviewable

[GBuella]

FYI, ctree tests: https://github.com/pmem/nvml/blob/master/src/test/obj_ctree/obj_ctree.c

[igchor]

Review status: all files reviewed at latest revision, 9 unresolved discussions.

---

src/libpmemfile-posix/block_array.c, line 478 at r5 (raw file):

Previously, GBuella (Gabor Buella) wrote…

If inserting a block, then aborting the transaction?

If inserting a block fails, then aborting the transaction. It looked the same for ctree_insert.

---

src/libpmemfile-posix/offset_mapping.c, line 40 at r5 (raw file):

Previously, sarahjelinek (Sarah Jelinek) wrote…

s/defiend/defined

Done.

---

src/libpmemfile-posix/offset_mapping.c, line 60 at r5 (raw file):

Previously, GBuella (Gabor Buella) wrote…

I see this business of representing ranges with ranges of different overlapping ranges (sorry if I don't make any sense) is pretty complicated to implement, are there separate unit tests just for the tree? It might be useful. Is it correct with silly corner cases?

I didn't have enough time to write proper tests, which could be put in the repo yet. But I will try to create some tests today.

---

src/libpmemfile-posix/offset_mapping.c, line 180 at r5 (raw file):

Previously, GBuella (Gabor Buella) wrote…

Another little diagram here would be cool

Done.

---

src/libpmemfile-posix/offset_mapping.c, line 195 at r5 (raw file):

Previously, GBuella (Gabor Buella) wrote…

And a little diagram here

Done.

---

Comments from Reviewable

[GBuella]

Review status: 8 of 9 files reviewed at latest revision, 9 unresolved discussions.

---

src/libpmemfile-posix/offset_mapping.c, line 219 at r6 (raw file):

   *
   * entry mapping offset 48k to block is NULL, and there is
   * no non-NULL entry to the left, so in this case BBB->prev

So, is this supposed to find the first block? But that prev pointer must be null in this case anyways, or not? We only store initialized blocks here?

Also, there is this field in struct pmemfile_vinode: https://github.com/pmem/pmemfile/blob/master/src/libpmemfile-posix/inode.h#L97

---

src/libpmemfile-posix/offset_mapping.c, line 227 at r6 (raw file):

          e++;
      else if (!e->internal) {
          return PF_RW(m->pfp, e->data.block->prev);

So can this return non-null?

---

Comments from Reviewable

[GBuella]

Review status: 8 of 9 files reviewed at latest revision, 8 unresolved discussions.

---

src/libpmemfile-posix/block_array.c, line 478 at r5 (raw file):

Previously, igchor wrote…

If inserting a block fails, then aborting the transaction. It looked the same for ctree_insert.

Ye, I see it was the same confusing thing in the original version as well.

---

Comments from Reviewable

[GBuella]

Review status: 8 of 9 files reviewed at latest revision, 5 unresolved discussions.

---

src/libpmemfile-posix/offset_mapping.c, line 219 at r6 (raw file):

Previously, GBuella (Gabor Buella) wrote…

So, is this supposed to find the first block? But that `prev` pointer must be null in this case anyways, or not? We only store initialized blocks here? Also, there is this field in struct pmemfile_vinode: https://github.com/pmem/pmemfile/blob/master/src/libpmemfile-posix/inode.h#L97

OK, now I understand it finds the first block in a range, not the first block in the whole file.

---

src/libpmemfile-posix/offset_mapping.c, line 227 at r6 (raw file):

Previously, GBuella (Gabor Buella) wrote…

So can this return non-null?

I get it now.

---

Comments from Reviewable

[krzycz]

Reviewed 1 of 1 files at r6. Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

Comments from Reviewable

[igchor]

To make offset_mapping test compile I had to add: #ifdef __cplusplus extern "C" { #endif

to offset_mapping.h and also define COMPILE_ERROR_ON to do nothing with c++.

If you have any ideas how to make this test compile without doing that, I will change that.

---

Review status: 3 of 13 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[GBuella]

Review status: 3 of 13 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

src/compiler_utils.h, line 69 at r8 (raw file):

#ifdef __cplusplus
#define COMPILE_ERROR_ON(cond)

We do use C++11, so this could be: #define COMPILE_ERROR_ON(cond) static_assert(!cond, "assertion failure") , or something like that. Some people have problem with using static_assert in C, even though it is supported pretty much everywhere -- and the NVML team instead invented their own wheel with opposite semantics...

But really noone should object to using it in C++, since it does not count as an extension since C++11.

---

Comments from Reviewable

[GBuella]

Reviewed 3 of 5 files at r7, 7 of 7 files at r8. Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[krzycz]

Reviewed 3 of 5 files at r7, 4 of 7 files at r8. Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[GBuella]

Review status: 7 of 36 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

src/libpmemfile-posix/offset_mapping.h, line 45 at r9 (raw file):

/* branching factor is 2^N_CHILDREN_POW */
#define N_CHILDREN_POW 4

I think these two constants should not be in the header file, and test should not know about these details. Also, perhaps some thing would be easier if this offset_mapping would just be a general mapping from 64 bit integers to other 64 bit integers. The tests include C headers, which were not meant to be compiled as C++, but testing a general map of integers should be possible with out using pmemfile_block_desc, pmemobj_oid, and such in tests.

---

Comments from Reviewable

[igchor]

Review status: 7 of 36 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

src/libpmemfile-posix/offset_mapping.c, line 162 at r5 (raw file):

Previously, GBuella (Gabor Buella) wrote…

For the curious: I realized the compiler can't do strength reduction on the division here, since divisor comes form a variable -> doesn't know it is a power of two. I compiled this, and there is a `div` instruction in the middle of this loop. I managed to get rid of the division by using this uglier form: ``` int bit_index = __builtin_ctzll(range); /* cuz I know it has a single bit set, but the compiler doesn't know */ while (entry->internal) { children = entry->data.children; bit_index -= N_CHILDREN_POW; entry = children + ((offset >> bit_index) & ((1 << N_CHILDREN_POW) - 1)); } ```

Ok, I've used this idea.

---

src/libpmemfile-posix/offset_mapping.h, line 45 at r9 (raw file):

Previously, GBuella (Gabor Buella) wrote…

I think these two constants should not be in the header file, and test should not know about these details. Also, perhaps some thing would be easier if this offset_mapping would just be a general mapping from 64 bit integers to other 64 bit integers. The tests include C headers, which were not meant to be compiled as C++, but testing a general map of integers should be possible with out using `pmemfile_block_desc`, `pmemobj_oid`, and such in tests.

Okay, I could write test which wouldn't use this constants but in this case it would be harder to test some corner cases. Also if my algorithm would just map integers I wouldn't be able to return block->prev as in line 233 in offset_mapping.c It could be done by the caller like this:

struct pmemfile_block_desc *block = block_find_closest( offset ); if (block->offset > offset) block = PF_RW(pfp, block->prev);

But this additional check could probably decrease performance.

---

Comments from Reviewable

[krzycz]

Reviewed 29 of 29 files at r9. Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[igchor]

Review status: 5 of 15 files reviewed at latest revision, 3 unresolved discussions.

---

src/compiler_utils.h, line 69 at r8 (raw file):

Previously, GBuella (Gabor Buella) wrote…

We do use C++11, so this could be: `#define COMPILE_ERROR_ON(cond) static_assert(!cond, "assertion failure")` , or something like that. Some people have problem with using static_assert in C, even though it is supported pretty much everywhere -- and the NVML team instead invented their own wheel with opposite semantics... But really noone should object to using it in C++, since it does not count as an extension since C++11.

I no longer compile c sources as c++ so it's not necessary now.

---

Comments from Reviewable

[marcinslusarz]

Reviewed 1 of 5 files at r4, 1 of 5 files at r7, 1 of 7 files at r8, 5 of 33 files at r10. Review status: 9 of 14 files reviewed at latest revision, 7 unresolved discussions.

---

tests/posix/CMakeLists.txt, line 167 at r10 (raw file):

if(FAULT_INJECTION)
    target_sources(file_offset_mapping PRIVATE

Please always use tabs in cmake files.

---

tests/posix/offset_mapping/offset_mapping.cpp, line 60 at r10 (raw file):

      return 1;

  uint64_t result = x;

If you would start from 1 instead of x you would not need to have a special case of y==0.

---

tests/posix/offset_mapping/offset_mapping_wrapper.c, line 1 at r10 (raw file):

#include "layout.h"

Missing copyright and license.

---

tests/posix/offset_mapping/offset_mapping_wrapper.h, line 1 at r10 (raw file):

#ifndef OFFSET_MAPPING_WRAPPER_H

Missing copyright and license.

---

Comments from Reviewable

[igchor]

Review status: 9 of 14 files reviewed at latest revision, 7 unresolved discussions.

---

tests/posix/CMakeLists.txt, line 167 at r10 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

Please always use tabs in cmake files.

Done.

---

tests/posix/offset_mapping/offset_mapping.cpp, line 60 at r10 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

If you would start from 1 instead of x you would not need to have a special case of y==0.

Done.

---

tests/posix/offset_mapping/offset_mapping_wrapper.c, line 1 at r10 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

Missing copyright and license.

Done.

---

tests/posix/offset_mapping/offset_mapping_wrapper.h, line 1 at r10 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

Missing copyright and license.

Done.

---

Comments from Reviewable

[marcinslusarz]

Reviewed 4 of 4 files at r11. Review status: 13 of 14 files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[marcinslusarz]

---

Reviewed 1 of 33 files at r10. Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[krzycz]

---

Reviewed 1 of 7 files at r8, 5 of 33 files at r10. Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[krzycz]

Reviewed 1 of 33 files at r10, 1 of 4 files at r11. Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[GBuella]

---

Reviewed 2 of 29 files at r9, 6 of 33 files at r10, 4 of 4 files at r11. Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable