[WASM] Add NonMaxSuppressionV5 Op

[lina128]

Note: Tested with latest core at e5a0c84, need to bump core version.

This change is 

[nsthorat]

Really nice work! Will stamp and merge after these minor comments :)

Reviewed 5 of 5 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov, @lina128, and @nsthorat)

---

tfjs-backend-wasm/src/cc/kernels/NonMaxSuppressionV5.cc, line 35 at r1 (raw file):

struct Result {
  int32_t* selected_indices;
  size_t selected_indices_size;

are selected_indices_size and selected_scores_size the same? If so just have a single return value for the size?

---

tfjs-backend-wasm/src/cc/kernels/NonMaxSuppressionV5.cc, line 100 at r1 (raw file):

  struct Candidate {
    int32_t box_index;

this can be size_t instead of int32_t

---

tfjs-backend-wasm/src/cc/kernels/NonMaxSuppressionV5.cc, line 102 at r1 (raw file):

    int32_t box_index;
    float score;
    int32_t suppress_begin_index;

this can be size_t instead of int32_t

---

tfjs-backend-wasm/src/cc/kernels/NonMaxSuppressionV5.cc, line 116 at r1 (raw file):

  // Filter out boxes that are below the score threshold and also maintain
  // the order of boxes by scores.
  for (int i = 0; i < num_boxes; i++) {

use size_t instead of int

---

tfjs-backend-wasm/src/cc/kernels/NonMaxSuppressionV5.cc, line 118 at r1 (raw file):

  for (int i = 0; i < num_boxes; i++) {
    if (scores[i] > score_threshold) {
      candidate_priority_queue.emplace(Candidate({i, scores[i], 0}));

can you make a named constant for the 0 here?

---

tfjs-backend-wasm/src/cc/kernels/NonMaxSuppressionV5.cc, line 150 at r1 (raw file):

    // iou_threshold, we simply ignore the candidate.
    bool ignore_candidate = false;
    for (int j = selected_indices.size() - 1;

use size_t instead of int

[dsmilkov]

Reviewed 2 of 5 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov and @lina128)

---

tfjs-backend-wasm/src/cc/kernels/NonMaxSuppressionV5.cc, line 123 at r1 (raw file):

  // If soft_nms_sigma is 0, the outcome of this algorithm is exactly same as
  // before.

can you extract the entire logic to a shared util non_max_suppression_util.cc which gets called from both v3 and v5, where v3 passes a soft_nms_sigma=0? this way we don't have to maintain two separate impls.

[dsmilkov]

Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov and @lina128)

---

tfjs-backend-wasm/src/cc/kernels/NonMaxSuppressionV5.cc, line 123 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

can you extract the entire logic to a shared util non_max_suppression_util.cc which gets called from both v3 and v5, where v3 passes a soft_nms_sigma=0? this way we don't have to maintain two separate impls.

See interpolate_bilinear_impl.h/cc and how we use it in with ResizeBilinear.cc and CropAndResize.cc

[dsmilkov]

This is awesome!

Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov and @lina128)

[lina128]

Hi @nsthorat, thank you for the very detailed review! I changed the selected_indices from int_32* to size_t*, but there's runtime error 'Access memory out of bound'. Addressed other comments otherwise.

[lina128]

Hi @dsmilkov, thank you for the great suggestion! Extract logic to shared file, please re-review the PR. Thank you!

[dsmilkov]

Awesome work. Just a couple comments but no need for a follow up review, thus LGTM!

Reviewed 1 of 5 files at r1, 10 of 10 files at r2.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @lina128)

---

tfjs-backend-wasm/src/cc/kernels/NonMaxSuppressionV3.cc, line 31 at r2 (raw file):

// Structure to store the result of the kernel. In this case we give js a
// a pointer in memory where the result is stored and how big it is.
struct Result {

remove this data structure since we are using the one in the shared impl

---

tfjs-backend-wasm/src/cc/kernels/NonMaxSuppressionV5.cc, line 33 at r2 (raw file):

// Structure to store the result of the kernel. In this case we give js a
// a pointer in memory where the result is stored and how big it is.
struct Result {

same here, remove this data structure since we are using the one in the shared impl

---

tfjs-backend-wasm/src/kernels/NonMaxSuppressionV3.ts, line 69 at r2 (raw file):

  const {pSelectedIndices, selectedSize} =
      parseResultStruct(backend, resOffset);

to avoid memleak, call backend.wasm._free(pSelectedScores); here since V3 doesn't return the scores vector.

[dsmilkov]

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @lina128)

[lina128]

  const {pSelectedIndices, selectedSize} =
      parseResultStruct(backend, resOffset);

to avoid memleak, call backend.wasm._free(pSelectedScores); here since V3 doesn't return the scores vector.

Hi @dsmilkov, given that parseResultStruct has backend.wasm._free(resOffset), do we really need to free pSelectedScores? I thought free(resOffset) will free all the memory allocated for the NonMaxSuppressionResult.

[dsmilkov]

Yes we do need another free since backend.wasm._free(resOffset) will only remove the 4 bytes allocated for the pointer address of pSelectedScores, but it will not remove the array of values that were allocated on the heap that the pointer points to, thus you need explicit removal of that. The reason why you do not need to do the same with pSelectedIndices is because we wrap that in a Tensor object, which gets globally tracked and later gets disposed by the tidy mechanisms when it gets out of scope.

[lina128]

Yes we do need another free since backend.wasm._free(resOffset) will only remove the 4 bytes allocated for the pointer address of pSelectedScores, but it will not remove the array of values that were allocated on the heap that the pointer points to, thus you need explicit removal of that. The reason why you do not need to do the same with pSelectedIndices is because we wrap that in a Tensor object, which gets globally tracked and later gets disposed by the tidy mechanisms when it gets out of scope.

Thank you for the detailed explanation Daniel! Now I know how it works. :) Done.

[dsmilkov]

Reviewed 6 of 6 files at r3.
Reviewable status: complete! 1 of 1 approvals obtained

[dsmilkov]

Thanks for adding a comment on top of that line! Nice work Na!