Skip to content

refactor: For MutableCSR, Let degree/cap list directly open in container and avoid copy in dump#267

Merged
zhanglei1949 merged 2 commits into
alibaba:mainfrom
zhanglei1949:zl/ref-csr-cap-degree
Apr 23, 2026
Merged

refactor: For MutableCSR, Let degree/cap list directly open in container and avoid copy in dump#267
zhanglei1949 merged 2 commits into
alibaba:mainfrom
zhanglei1949:zl/ref-csr-cap-degree

Conversation

@zhanglei1949
Copy link
Copy Markdown
Member

Fix #265

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses #265 by refactoring MutableCsr to open and dump the degree (.deg) and capacity (.cap) lists directly via OpenContainer, reducing redundant O(V) element-wise copies during open/dump and aligning the approach with how the neighbor list is handled.

Changes:

  • Refactor MutableCsr::open_internal() and dump() to use OpenContainer()/IDataContainer::Dump() directly for .deg and .cap.
  • Replace the previous std::atomic<int>-backed degree buffer usage with an int buffer and atomic builtins in selected write paths.
  • Minor formatting tidy in a test and ignore .aone_copilot/ in .gitignore.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.

File Description
src/storages/csr/mutable_csr.cc Open/dump degree & capacity lists via containers; adjust degree access patterns across CSR ops.
include/neug/storages/csr/mutable_csr.h Update member containers and degree access in put_edge()/views to use the new degree/cap containers.
tests/execution/test_value.cc Formatting-only change to an EXPECT_DEATH call.
.gitignore Add .aone_copilot/ ignore entry.
Comments suppressed due to low confidence (2)

src/storages/csr/mutable_csr.cc:448

  • delete_edge bounds-checks offset against sz_arr[src] using a plain read from the degree array. Since degrees are incremented with atomic RMWs in put_edge/batch_put_edges, this non-atomic read can race with concurrent inserts/deletes and is undefined behavior in C++. Use an atomic load (e.g., std::atomic_ref<int>(sz_arr[src]).load(std::memory_order_relaxed) or __atomic_load_n) or ensure higher-level synchronization guarantees no concurrent degree modifications when calling this API.
    THROW_INVALID_ARGUMENT_EXCEPTION("src out of bound or offset out of bound");
  }
  nbr_t* nbrs = reinterpret_cast<nbr_t**>(adj_list_buffer_->GetData())[src];
  if (nbrs == nullptr) {

src/storages/csr/mutable_csr.cc:558

  • In batch_put_edges, degrees are read/written via plain int accesses in earlier loops, but later updated with __atomic_fetch_add(&sz_arr[src], ...). Mixing atomic and non-atomic accesses to the same locations is undefined behavior and can lead to inconsistent degrees under concurrent readers/writers. Consider switching all degree accesses in this method to std::atomic_ref<int> (or __atomic_* builtins) for consistency, or clearly document/enforce that batch_put_edges runs single-threaded with no concurrent readers.
    nbr.data = data;
    nbr.timestamp.store(ts);
    added_edge_num++;
  }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/storages/csr/mutable_csr.cc
Comment thread include/neug/storages/csr/mutable_csr.h
Comment thread src/storages/csr/mutable_csr.cc
@zhanglei1949 zhanglei1949 changed the title refactor: For MutableCSR, Let degree list directly open in container and avoid copy in dump refactor: For MutableCSR, Let degree/cap list directly open in container and avoid copy in dump Apr 21, 2026
@zhanglei1949 zhanglei1949 force-pushed the zl/ref-csr-cap-degree branch from 2ed3596 to 14f8432 Compare April 22, 2026 02:53
format

minor fix

no need to use atomic for degree

remove need_cap_list
@zhanglei1949 zhanglei1949 force-pushed the zl/ref-csr-cap-degree branch from 14f8432 to c964e58 Compare April 22, 2026 04:07
@zhanglei1949 zhanglei1949 merged commit c4d76f5 into alibaba:main Apr 23, 2026
16 checks passed
@lnfjpt lnfjpt mentioned this pull request May 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MutableCsr: Avoid redundant O(V) copy for degree/capacity lists during open and dump

3 participants