Merging PMDebugger into Pmemcheck #83
Conversation
There was a problem hiding this comment.
- Please, squash your commits.
- Please, format your code uniformly and try to follow the same coding style as in the existing code.
Yes, it matters. - Is there any documentation on what has changed and why?
- Any unit tests for that?
Reviewed 2 of 3 files at r1.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @dibang2008)
pmemcheck/pmc_include.h, line 35 at r1 (raw file):
Bool is_delete;
I'd suggest to move it to the end of the struct. Otherwise you waste 7 bytes for padding in each instance of this struct. And if you preallocate an array of 100000000 such structs it's quite a lot of wasted memory.
In general, it would be nice to arrange elements in this and arr_md structures in a way that we don't waste too much memory.
pmemcheck/pmc_main.c, line 63 at r1 (raw file):
100000000UL
Could it be configurable?
|
@krzycz this is context of https://dl.acm.org/doi/abs/10.1145/3445814.3446744?sid=SCITRUS @dibang2008 has agreed to upstream those changes back. |
… why; (3) move is_delete to the end of the struct and change 100000000UL to 1000UL
|
I have done following things according to your comments:
For unit tests, I have used the existing tests in Pmemcheck to verify my code (perl tests/vg_regtest pmemcheck) and I am not sure that is suitable for you. If you want more comprehensive unit tests, please give me more details (I am not familiar with this area). Thanks |
There was a problem hiding this comment.
I've done an initial high-level pass of this PR. In general, I think this change should be done behind an abstraction that hides the details of the two data structures used for pmem stores. Right now, all this code is intertwined and hard to read for me.
I'm also worried about the maintainability of the implementation if there are two data structures that need to be updated - a nice abstraction should be able to solve this problem.
pmemcheck/pmc_include.h
Outdated
| @@ -32,6 +32,7 @@ struct arr_md { | |||
|
|
|||
| /** Single store to memory. */ | |||
| struct pmem_st { | |||
| Bool is_delete; | |||
There was a problem hiding this comment.
yes, please ensure that is data structure is as compact as possible. If the Bool type is 1 byte, then, as @krzycz pointed out, this wastes tons of memory for alignment.
There was a problem hiding this comment.
I removed this 'is_delete' flag and now I use 'address size = 0' to mark a persistent memory info as deleted
pmemcheck/pmc_main.c
Outdated
| @@ -308,12 +309,28 @@ print_multiple_stores(void) | |||
| static void | |||
| print_store_stats(void) | |||
| { | |||
| /* print store states of the array*/ | |||
|
OK, I will try to solve these problems next week, because I have to prepare my dissertation oral defense this week |
|
No worries, take your time. |
…move the is_delete flag; (4) abstract the process of inserting data;
pmemcheck/pmc_main.c
Outdated
| @@ -294,16 +339,33 @@ print_multiple_stores(void) | |||
| static void | |||
| print_store_stats(void) | |||
| { | |||
| /* print store states of the array */ | |||
| struct pmem_st *tmp; | |||
| for (int s_index=0; s_index<=pInfo.info_array.m_index; s_index++) { | |||
There was a problem hiding this comment.
make a temporary variable for pInfo.info_array. This will allow you to shorten this code quite a bit.
pmemcheck/pmc_main.c
Outdated
| for (int s_index=0; s_index<=pInfo.info_array.m_index; s_index++) { | ||
| if (cmp_with_arr_minandmax(region, s_index) != -1) { | ||
| for (int i = pInfo.info_array.m_data[s_index].start_index; i < pInfo.info_array.m_data[s_index].end_index; i++){ | ||
| //VG_(umsg)("remove before=%d\n",VG_(OSetGen_Size)(curr_node->pmem_stores)); |
There was a problem hiding this comment.
please remove this (and other comments like this)
pmemcheck/pmc_main.c
Outdated
| Bool valid_flush = False; | ||
|
|
||
| for(int s_index=0; s_index <= pInfo.info_array.m_index; s_index++) { | ||
| if (flush_max >= pInfo.info_array.m_data[s_index].max_addr && pInfo.info_array.m_data[s_index].min_addr >= flush_info.addr && |
There was a problem hiding this comment.
use a temp variable for pInfo.info_array.m_data[s_index]
|
Thanks for cleaning up this code. Looks much nicer now, but there are still some stylistic changes that make it a bit hard to read. I've pointed out a few in my comments. I'm still a bit worried that this change is going to make pmemcheck a bit harder to maintain - so I'll try to think of a way to mitigate that. |
|
I have fixed these issues. Thanks for your valuable suggestions! |
There was a problem hiding this comment.
We are going to run regression testing over this and I also asked for someone else in my team to take a look at your patch.
Thanks.
Reviewable status: 1 of 4 files reviewed, 6 unresolved discussions (waiting on @dibang2008 and @krzycz)
pmemcheck/pmc_include.h, line 20 at r4 (raw file):
/** Metadata structure for store information array */ struct arr_md { UWord end_index; //Index of current free metadata
can you explain in a comment how the end_index and start_index fields work? And in general, what's the relation between this structure and pmem_stores in the pmem_stores_array.
If my understanding is correct, there can be more than one pmem_st per arr_md, and this structure has a slice into the stores array. Would it be possible to reorganize it so that this is more obvious, like so:
struct pmem_stores_span {
Addr min_addr;
Addr max_addr;
Vec<pmem_st> stores;
enum flushed_state state;
};
struct pmem_stores_array {
Vec<pmem_stores_span> store_spans;
}
where Vec<T> is just a dynamic array of elements of type T.
Or am I wrong entirely?:D
pmemcheck/pmc_main.c, line 901 at r4 (raw file):
if (cmp_with_arr_minandmax(store, s_index) != -1) { for (int i = pInfo.info_array.m_data[s_index].start_index; i < pInfo.info_array.m_data[s_index].end_index; i++) { if (LIKELY(pInfo.info_array.pmem_stores[i].size != 0)) {
please simplify control flow:
if (... == 0)
continue;
existing = ...;
pmemcheck/pmc_main.c, line 1312 at r4 (raw file):
struct pmem_st *being_fenced = NULL; for(int s_index=0; s_index <= pInfo.info_array.m_index; s_index++) { if (pInfo.info_array.m_data[s_index].state != ALL_FLUSHED) {
please use a temp variable for pInfo.info_array.m_data[s_index]
And please try to keep lines to under 80.
|
Yes, your understanding is correct and we can make our structures more obvious. But there is one problem: a dynamic structure (such as Oset in Valgrind) may introduce extra overheads because a dynamic structure requires reallocations, deletions, and rebalancing. These overheads can be very large as the number of instructions increase. We can try a dynamic structure but we need to find a more lightweight structure supported by Valgrind (The Oset structure is not a good choice). Any suggestion? Thanks. |
|
Well, you are already using a reallocating array - so my suggestion would be to wrap it in a small abstraction called "Ovec" or something like that. Btw, we've run regression testing over PMDK using valgrind from this patch and many of our tests have failed with this error: Can you reproduce this issue on your end? |
|
Can you send me the regression testing? It can help me reproduce this bug. I am not sure if the regression testing can be open-source. If not, can you give some hints of input data? I will try to reproduce this bug. Thanks :). |
|
We've simply ran the entire PMDK test suite under this version of pmemcheck. The specific output example was from obj_basic_integration TEST1. You can run it by going into the directory of that test ( |
|
I also have a bug for this test but it is not the same as yours. It is as follow: This bug has been fixed by aligning |
This change is