vmware / vmware/splinterdb

Audit: Review ROUTING_FPS_PER_PAGE 4096, and multiple uses of hard-coded 32 in routing_filter.c

Open
#242 1 comment 0 reactions 1 assignee View on GitHub

Nobody has claimed this yet.

code-cleanup documentation
Dominant language
C
Stars
732
Forks
67
PR merge metrics
No merged PRs in 30d

Description

This issue was identified during code-walk thru of PR #230. There are these references in routing_filter.c, which need a better understanding, commenting in the file.

24 #define ROUTING_FPS_PER_PAGE 4096

During discussion, it appeared that 4096 is some sort of "upper-bound" on number of routing filters, but it wasn't clear whether this should be related in any way to page size [etc.]

 399       MATRIX_ROWS * MATRIX_COLS + // matrix
 400       ROUTING_FPS_PER_PAGE +      // fp_buffer
 401       ROUTING_FPS_PER_PAGE +      // old_fp_buffer
 402       ROUTING_FPS_PER_PAGE / 32;  // encoding_buffer
 413    old_fp_buffer = fp_buffer + ROUTING_FPS_PER_PAGE;
 414    encoding_buffer = (uint64 *)(old_fp_buffer + ROUTING_FPS_PER_PAGE);
 415    memset(encoding_buffer, 0xff, ROUTING_FPS_PER_PAGE / 32 * sizeof(uint32));

There are many more instances of hard-coded '32' ... and it's not clear what the math is all about here. E.g.

 147 routing_get_index(uint32 fp,
 148                   size_t index_remainder_and_value_size)
 149 {
 150    return index_remainder_and_value_size == 32
 151       ? 0 : fp >> index_remainder_and_value_size;
 239       // ffs returns the index + 1 ALEX: I think that's what we want though.
 240       bit_offset = __builtin_ffs(encoding_word) - 1;
 241       *end = 32 * word + bit_offset;

 446    for (uint32 new_fp_no = 0; new_fp_no < num_new_fp; new_fp_no++) {
 447       new_fp_arr[new_fp_no] >>= 32 - cfg->fingerprint_size;
 448       new_fp_arr[new_fp_no] <<= value_size;

  698       platform_assert(cfg->fingerprint_size + value_size <= 32);

 811    uint32 fp = hash(slice_data(key), slice_length(key), seed);
 812    fp >>= 32 - cfg->fingerprint_size;

And we should find a way to give a semantic #define for 32, and use that consistently where applicable in this file.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.