Implementation of mechanisms for synchronisation of memory access at the knowledge level #483
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…on, transaction, epoch management)
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 84f8bac in 3 minutes and 8 seconds
More details
- Looked at
322lines of code in8files - Skipped
0files when reviewing. - Skipped posting
16drafted comments based on config settings.
1. sc-memory/sc-core/src/sc-store/sc-transaction/sc_transaction_manager.c:18
- Draft comment:
sc_transaction_queue and sc_monitor members are used without being allocated; they need proper initialization. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. sc-memory/sc-core/tests/units/common/test_sc_transaction_core.cpp:50
- Draft comment:
Test references 'manager.log_head', but the transaction manager structure does not include a log_head field. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
Without seeing the actual definition of transaction_manager from the header file, I cannot verify if log_head exists or not. The fact that it's used consistently across multiple tests suggests it probably does exist. The comment makes a strong claim about the struct's definition without providing evidence.
I could be wrong - maybe the field really is missing and all these tests are incorrect. The header file contents would resolve this.
Given that this is a newly added test file and it consistently uses this field in multiple places, it's more likely that the field exists and the comment is incorrect.
Delete the comment since we don't have strong evidence that the field is actually missing, and the consistent usage in tests suggests it likely exists.
3. sc-memory/sc-core/src/sc-store/sc-transaction/sc_transaction_manager.c:17
- Draft comment:
Potential bug: sc_transaction_queue and sc_monitor are used without prior allocation. Allocate these members before calling their init functions. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment seems incorrect because sc_mem_new() allocates the entire structure. These members are part of the structure, not pointers that need separate allocation. The initialization functions likely just set up the internal state of these already-allocated members. Without seeing the struct definition we can't be 100% certain, but the pattern suggests these are embedded struct members, not pointers.
I don't have access to the struct definition or the init function implementations, so I can't be completely certain about the memory layout or initialization requirements.
The code follows a common pattern where structure members are allocated as part of the containing struct and then initialized. The destroy functions also handle these members appropriately, suggesting they are properly managed.
The comment appears incorrect - the members are likely allocated as part of the structure and just need initialization, which is being done correctly.
4. sc-memory/sc-core/src/sc-store/sc-transaction/sc_transaction.h:18
- Draft comment:
Several transaction functions (add_element, commit, rollback, validate, merge, apply, clear) are declared but not implemented. Mark them as TODO or provide stubs. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
This is a header file which typically only contains declarations. Implementation would be in a .c file. It's normal and expected for header files to only have declarations. The functions already have clear documentation comments explaining their purpose. Adding TODO markers would be redundant since the implementation naturally belongs in a separate file.
Maybe the reviewer knows something we don't - perhaps there should be inline implementations or this is part of a larger change that's missing implementation files?
Even if implementation files are missing, that would be caught by the build system when linking. Header files containing only declarations is standard C practice.
The comment should be deleted as it suggests unnecessary changes to a properly structured header file.
5. sc-memory/sc-core/src/sc-store/sc-transaction/sc_transaction_manager.h:21
- Draft comment:
Declared transaction manager functions (transaction_add and transaction_execute) are not implemented. Consider adding TODO comments or implementations. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
Header files are meant to declare the interface, not provide implementations. The implementations would naturally go in a separate source file. This is standard C programming practice. The comment seems to misunderstand the purpose of header files. Additionally, each function has a clear comment describing its purpose.
Maybe there's evidence elsewhere that shows these functions are actually missing their implementations in the source files? Maybe this is part of a larger change that removed implementations?
Even if implementations are missing, that would be caught by the build process when linking. We don't have access to the full codebase to verify, and build errors would catch this anyway.
This comment should be deleted as it misunderstands the purpose of header files and raises an issue that would be caught by the build process anyway.
6. sc-memory/sc-core/tests/units/common/test_sc_transaction_core.cpp:14
- Draft comment:
Test API mismatch: The test uses undefined functions (log_version_change, commit_transaction, rollback_transaction) and mismatched structure fields (current_transaction, log_head) that don’t align with the new sc_transaction_manager API. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
Since this is a new test file, it's using an API defined in sc_transaction_manager.h which we can't see. Without seeing the header file, we can't verify if these functions and fields actually exist. The comment could be correct, but we don't have enough context to be certain. The default should be to remove comments when we lack sufficient evidence.
I might be wrong because the reviewer could have checked the header file and found these functions/fields are indeed missing. Also, the extern "C" block suggests this is interfacing with C code where API mismatches would be particularly problematic.
While those are valid concerns, without seeing the actual API definition, we can't be certain this is a real issue. Following our principle of needing strong evidence, we should err on the side of removing the comment.
Delete the comment because we lack sufficient evidence to confirm there is actually an API mismatch. The header file contents would be needed to verify this claim.
7. sc-memory/sc-core/src/sc-store/sc-transaction/sc_transaction.c:24
- Draft comment:
Minor Formatting Issue: The file is missing a trailing newline at the end. Please add a newline (LF) at the end of the file to adhere to common style guidelines. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While having a trailing newline is indeed a common convention, this is something that should be handled by automated tooling or linters rather than manual code review comments. It's a very minor stylistic issue that doesn't affect functionality. The rules state not to make comments that are obvious or unimportant.
The trailing newline convention does serve a purpose in Unix-like systems and some tools. Missing it could cause issues with certain tools or commands.
While true, this is still too minor for a PR comment and should be handled by automated tools. If it's important for the project, it should be enforced by CI/CD.
Delete this comment as it's too minor and should be handled by automated tooling rather than manual review.
8. sc-memory/sc-core/src/sc-store/sc-transaction/sc_transaction_manager.c:58
- Draft comment:
A newline at the end of the file is missing. Adding a newline can prevent potential warnings in some editors and tools. This is a trivial typographical issue. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is purely informative and does not provide a specific code suggestion or highlight a potential issue that needs addressing. It does not align with the rules provided, as it doesn't suggest a change or confirm an intention. Therefore, it should be removed.
9. sc-memory/sc-core/src/sc-store/sc_element.h:30
- Draft comment:
Line 30: The sentence 'It used just for unify working with node and arc elements.' is grammatically unclear. Consider rephrasing it to 'It is used to unify operations on node and arc elements.' - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. sc-memory/sc-core/src/sc-store/sc_element.h:31
- Draft comment:
Line 31: In the phrase 'check it type', 'it' should be 'its'. Please update to 'check its type'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. sc-memory/sc-core/src/sc-store/sc_element.h:38
- Draft comment:
Line 38: The sentence 'It stores as a one char for each of types.' is unclear. Consider rephrasing it to 'It stores one char for each type.' - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. sc-memory/sc-core/src/sc-store/sc_element.h:39
- Draft comment:
Line 39: The phrase 'Each node have a pointer to content assigned with it.' should be corrected to 'Each node has a pointer to content assigned to it.' - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. sc-memory/sc-core/src/sc-store/sc_element.h:40
- Draft comment:
Line 40: The sentence 'then node haven't any content data.' should be updated to 'then the node does not have any content data.' - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. sc-memory/sc-core/src/sc-store/sc_element.h:41
- Draft comment:
Line 41: The sentence 'Last part of node information is a pointers to the lists of input and outgoing sc-arcs.' is unclear. Consider revising it to 'The last part of node information consists of pointers to the lists of input and outgoing sc-arcs.' - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
15. sc-memory/sc-core/src/sc-store/sc_element.h:42
- Draft comment:
Line 42: The sentence 'Each list of arcs contains pointer to array of arcs pointers.' should be rephrased to 'Each list of arcs contains a pointer to an array of arc pointers.' - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
16. sc-memory/sc-core/src/sc-store/sc_element.h:45
- Draft comment:
Line 45: The phrase 'Each element store addr of begin and end arcs.' should be corrected to 'Each element stores the addresses of the beginning and end arcs.' - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_BrlN9NDJ4fz4Jyxw
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| #ifndef SC_TRANSACTION_H | ||
| #define SC_TRANSACTION_H | ||
|
|
||
| #include <sc-core/sc-container/sc_list.h> |
There was a problem hiding this comment.
Consider including "sc_element.h" so that the type sc_element is defined for sc_transaction_add_element.
| pthread_join(threads[i], nullptr); | ||
| } | ||
|
|
||
| EXPECT_EQ(manager.current_transaction, nullptr); |
There was a problem hiding this comment.
Test refers to manager.current_transaction but the structure defines current_sc_transaction; please ensure consistent naming.
| #include "sc-store/sc-base/sc_monitor_private.h" | ||
|
|
||
| typedef struct sc_transaction_manager { | ||
| sc_transaction*current_sc_transaction; |
There was a problem hiding this comment.
Consider adding a space between the * and current_sc_transaction (i.e., change sc_transaction*current_sc_transaction to sc_transaction* current_sc_transaction) for better readability.
Suggested change
| sc_transaction*current_sc_transaction; | |
| sc_transaction* current_sc_transaction; |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #483 +/- ##
==========================================
- Coverage 95.74% 95.42% -0.33%
==========================================
Files 237 242 +5
Lines 27369 27653 +284
Branches 1877 1919 +42
==========================================
+ Hits 26204 26387 +183
- Misses 1165 1266 +101 🚀 New features to boost your workflow:
|
…nges, implementation of adding the created element to the buffer
NikitaZotov
requested changes
Mar 29, 2025
|
|
||
| #include <sc-core/sc-base/sc_allocator.h> | ||
|
|
||
| sc_transaction * sc_transaction_new(sc_uint64 const * txn_id) |
There was a problem hiding this comment.
I think that an int pointer is not needed here
Comment on lines
17
to
34
| txn->elements = sc_mem_new(sc_list, 1); | ||
| if (txn->elements == NULL) | ||
| { | ||
| sc_mem_free(txn); | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
Suggested change
| txn->elements = sc_mem_new(sc_list, 1); | |
| if (txn->elements == NULL) | |
| { | |
| sc_mem_free(txn); | |
| return NULL; | |
| } |
There was a problem hiding this comment.
sc_list_init calls sc_mem_new for txn->elements
| txn->transaction_buffer = sc_mem_new(sc_transaction_buffer, 1); | ||
| if (txn->transaction_buffer == NULL) | ||
| { | ||
| sc_mem_free(txn->elements); |
There was a problem hiding this comment.
Suggested change
| sc_mem_free(txn->elements); |
| { | ||
| if (txn != NULL) | ||
| { | ||
| sc_list_clear(txn->elements); |
There was a problem hiding this comment.
Suggested change
| sc_list_clear(txn->elements); | |
| sc_list_destroy(txn->elements); |
| } sc_transaction; | ||
|
|
||
| sc_transaction * sc_transaction_new(sc_uint64 const * txn_id); | ||
| // create a new transaction |
There was a problem hiding this comment.
We use another style for code strings
There was a problem hiding this comment.
What exactly should be adjusted to match the required code style?
Comment on lines
56
to
146
| sc_addr * addr_copy = sc_mem_new(sc_addr, 1); | ||
| if (addr_copy == null_ptr) | ||
| return SC_FALSE; | ||
|
|
||
| *addr_copy = addr; | ||
|
|
||
| if (sc_list_push_back(buffer->new_elements, addr_copy) == null_ptr) | ||
| { | ||
| sc_mem_free(addr_copy); | ||
| return SC_FALSE; | ||
| } |
There was a problem hiding this comment.
It is preferable to store in such lists not a pointer to an sc-address, but rather the hash of the sc-address, cast to void *. To obtain the sc-address hash from the sc-address, use the SC_ADDR_LOCAL_TO_INT macro. This approach is more efficient and memory-friendly compared to using pointers to objects.
There was a problem hiding this comment.
Try to introduce this approach everywhere
| if (SC_ADDR_IS_EMPTY(addr)) | ||
| return SC_FALSE; | ||
|
|
||
| sc_element_version * new_version = sc_element_create_new_version(element, /* version_id */ 0, flags); // Add generation for id |
There was a problem hiding this comment.
See sc_monitor_table to learn how to generate id for new version
Comment on lines
22
to
23
| transaction_manager->transaction_queue = sc_mem_new(sc_queue, 1); | ||
| transaction_manager->monitor = sc_mem_new(sc_monitor, 1); |
There was a problem hiding this comment.
You cannot initialize these objects in this way. They are initialized using the corresponding initialization methods
There was a problem hiding this comment.
sc_monitor_init and sc_queue_init methods have no memory allocation (no sc_mem_new())
void sc_monitor_init(sc_monitor * monitor)
{
sc_mutex_init(&monitor->rw_mutex);
monitor->id = 1;
monitor->active_readers = 0;
monitor->active_writer = 0;
sc_queue_init(&monitor->queue);
sc_mutex_init(&monitor->ref_count_mutex);
monitor->ref_count = 0;
}
void sc_queue_init(sc_queue * queue)
{
queue->data = sc_mem_new(void *, INITIAL_CAPACITY);
queue->front = 0;
queue->back = -1;
queue->size = 0;
queue->capacity = INITIAL_CAPACITY;
}
|
|
||
| sc_transaction * sc_transaction_new(sc_uint64 const * txn_id) | ||
| { | ||
| if (txn_id == NULL) |
There was a problem hiding this comment.
Please, use null_ptr instead of NULL
Comment on lines
16
to
18
| #ifndef null_ptr | ||
| # define null_ptr NULL | ||
| #endif |
There was a problem hiding this comment.
Suggested change
| #ifndef null_ptr | |
| # define null_ptr NULL | |
| #endif |
IRomanchuk06
force-pushed
the
feat/sc_memory_knowledge_sync
branch
from
c6bc951 to
a418e36
Compare
…tely in sc-memory, monitor table belongs to manager
…basic: add to current)
… txn_manager tests
|
@ellipsis, give me a code review |
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to e71d400 in 3 minutes and 42 seconds. Click for details.
- Reviewed
2417lines of code in28files - Skipped
1files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. sc-memory/sc-core/src/sc-store/sc-transaction/sc_transaction.c:214
- Draft comment:
Clarify and document the ownership semantics for the new_element_data passed to sc_transaction_element_change (and similar functions). It’s unclear if the data is copied or merely referenced, which may affect memory management. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% This is a new file being added, so the comment is about changed code. The ownership semantics of parameters is important for memory management. However, without seeing sc_transaction_buffer_modified_add() implementation, we can't be certain if there's really an issue. The comment is speculative rather than pointing out a definite problem. The ownership semantics could be a real issue that needs documentation. Memory leaks or double-frees could occur if not properly documented. While memory management is important, the comment is speculative and asks for clarification rather than pointing out a definite issue. Per the rules, we should not keep comments that ask for clarification or explanation. Delete the comment since it violates the rule against asking for clarification or explanation from the PR author. If there is a real memory management issue, it should be stated definitively.
2. sc-memory/sc-core/src/sc-store/sc-transaction/sc_memory_transaction_manager.h:8
- Draft comment:
Typo in comment: "determinate amount" may be intended to be "determine the amount". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While this is technically correct and about a changed line since this is a new file, it's a very minor typo in a TODO comment. TODOs are temporary by nature and this doesn't affect code functionality. The meaning is still clear despite the typo. This seems like an unnecessarily nitpicky comment. The typo could indicate a non-native English speaker who might appreciate the correction. Clear communication in comments is important for maintainability. While clear communication is important, this typo is minor enough that the meaning is still obvious. The effort of addressing this comment outweighs its benefit. Delete the comment as it's too minor and doesn't affect code functionality or understanding.
3. sc-memory/sc-core/src/sc_memory.c:59
- Draft comment:
Typographical issue: consider revising the error message text. Specifically, "Error while initialize sc-transaction manager" could be changed to "Error while initializing the sc-transaction manager" for better grammatical correctness. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the grammatical correction would make the error message more polished, error messages are not user-facing documentation and the current message is still clear enough to understand. The suggestion is minor and doesn't affect functionality. We should focus PR comments on more substantial issues. The error message could be seen by developers during debugging, so having proper grammar could make the codebase appear more professional. The fix is very simple to make. While proper grammar is good, this is an internal error message that is clear enough as is. PR comments should focus on more impactful issues like logic, security, or major code quality problems. Delete this comment as it suggests a very minor grammatical change to an internal error message that doesn't impact functionality or code quality significantly.
4. sc-memory/sc-core/tests/units/transaction/test_sc_memory_transaction_operations.cpp:146
- Draft comment:
Typographical/copy-paste error: The assertion compares beg_el_after->outgoing_arcs_count with itself. It likely should compare the outgoing arcs count before and after the transaction (e.g. beg_el_before vs beg_el_after). - Reason this comment was not posted:
Marked as duplicate.
5. sc-memory/sc-core/tests/units/transaction/test_sc_memory_transaction_operations.cpp:147
- Draft comment:
Typographical/copy-paste error: The assertion compares beg_el_after->incoming_arcs_count with itself. It likely should be comparing the corresponding pre- and post-transaction values. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_VdKAHACSCq2EQ770
You can customize 
| } | ||
| } | ||
|
|
||
| sc_monitor_release_read(transaction_manager->monitor); |
There was a problem hiding this comment.
Release of a read lock is called unconditionally after the main loop in sc_transaction_handler without a matching acquire. This may lead to undefined behavior.
Suggested change
| sc_monitor_release_read(transaction_manager->monitor); |
| sc_monitor_table_get_monitor_for_addr(sc_memory_transaction_manager_get()->monitor_table, *beg_addr); | ||
| sc_monitor * end_monitor = | ||
| sc_monitor_table_get_monitor_for_addr(sc_memory_transaction_manager_get()->monitor_table, *end_addr); | ||
| sc_monitor_acquire_write_n(2, beg_monitor, end_monitor); |
There was a problem hiding this comment.
The monitors (beg_monitor and end_monitor) are acquired via sc_monitor_acquire_write_n before checking for null. Ensure that these monitors are verified not null prior to lock acquisition to avoid potential faults.
| sc_element * end_el_after = nullptr; | ||
| sc_storage_get_element_by_addr(end_addr, &end_el_after); | ||
|
|
||
| EXPECT_EQ(beg_el_after->outgoing_arcs_count, beg_el_after->outgoing_arcs_count); |
There was a problem hiding this comment.
Self‐comparison assertion; the test asserts that beg_el_after->outgoing_arcs_count equals itself. Likely the intent was to compare the before and after states (e.g. expecting an increment by 1).
Suggested change
| EXPECT_EQ(beg_el_after->outgoing_arcs_count, beg_el_after->outgoing_arcs_count); | |
| EXPECT_EQ(beg_el_after->outgoing_arcs_count, beg_el_before->outgoing_arcs_count + 1); |
| EXPECT_NE(transaction->elements, nullptr); | ||
| } | ||
|
|
||
| TEST_F(ScTransactionTest, TransactionElementNew) |
There was a problem hiding this comment.
The 'TransactionElementNew' test case is empty and does not verify any behavior. Consider adding assertions to validate the functionality.
| state.counters["rate"] = benchmark::Counter(iterations, benchmark::Counter::kIsRate); | ||
| if (state.thread_index() == 0) | ||
| { | ||
| while (ctxNum.load() != 0); |
There was a problem hiding this comment.
Busy-waiting on an atomic counter (ctxNum) is used to synchronize context teardown. This approach is suboptimal; consider using proper synchronization primitives (e.g. condition variables or barriers) to avoid CPU spinning.
NikitaZotov
force-pushed
the
feat/sc_memory_knowledge_sync
branch
from
e71d400 to
e6b3aef
Compare
IRomanchuk06
force-pushed
the
feat/sc_memory_knowledge_sync
branch
from
70e3a48 to
0911739
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Implement transaction management and element versioning for synchronized memory access in sc-machine, with comprehensive tests and benchmarks.
sc_memory_transaction_managerinsc_memory_transaction_manager.candsc_memory_transaction_manager.hfor managing transactions with functions likesc_memory_transaction_manager_initialize(),sc_memory_transaction_commit(), andsc_transaction_manager_transaction_execute().sc_threadinsc_thread.candsc_thread.h.sc_element_versionandsc_element_datainsc_element_version.candsc_element_version.hfor versioning elements.sc_version_segmentinsc_version_segment.candsc_version_segment.h.sc_memory_transaction_operations.candsc_memory_transaction_operations.hfor creating nodes, links, and arcs within transactions.sc_storage.candsc_storage_private.hto support element versioning and transaction operations.test_sc_memory_transaction_operations.cpp,test_sc_transaction.cpp,test_sc_transaction_buffer.cpp, andtest_sc_transaction_manager.cpp.benchmark/main.cppand related files to evaluate transaction performance.This description was created by
for e71d400. You can customize this summary. It will automatically update as commits are pushed.