age icon indicating copy to clipboard operation
age copied to clipboard

Review internal graph management, provide feedback, and regression tests to test memory management

Open jrgemignani opened this issue 3 years ago • 1 comments

The internal graph representation and management routines are in the source file age_global_graph.c.

For this task, please do the following -

  1. Review the above source code and try to get a general understanding of how the graph is represented and managed.
  2. Provide feedback, as necessary, for self documentation of that file.
  3. Provide feedback, as necessary, for potential improvements of the logic or representation, if found.
  4. Create regression tests, to test the memory management routines.

Task 4 will arguably be the most difficult as you will need to have a good understanding from task 1.

Additionally, you may need to look at age_graphid_ds.c and age_vle.c to get a more complete picture of the in memory organization.

Please comment on the task when you start and finish it, in addition to anything that you find along the way.

jrgemignani avatar Sep 07 '22 00:09 jrgemignani

Started the task.

rafsun42 avatar Oct 02 '22 17:10 rafsun42

Started the task

PragyanD avatar Oct 06 '22 20:10 PragyanD

Some feedback I can give so far:

  1. There is a typo at line 44.
  2. The load_vertex_hashtable and load_edge_hashtable functions have many common chunks. Would it be a good idea to refactor these two functions to separate the common chunks into a new function?

rafsun42 avatar Oct 10 '22 01:10 rafsun42

Finished reviewing the files. Will start working on the regression tests

PragyanD avatar Oct 14 '22 05:10 PragyanD

Wrote regression tests for the delete_global_graphs command. I would like to get feedback on the test script before I can mark this task as complete. Should I send a pull request?

While testing, I found an issue in the delete_specific_GRAPH_global_contexts function. I opened a issue #332. If you think it is a valid issue, I can start working on the fix.

rafsun42 avatar Oct 14 '22 22:10 rafsun42

I have fixed that issue, it was in code that I wrote so rather easy for me to find and correct it. A patch will be coming soon to address that issue. After you double check that the fix works, using your regression tests, go ahead and send a PR.

jrgemignani avatar Oct 17 '22 23:10 jrgemignani

Some feedback I can give so far:

  1. There is a typo at line 44.
  2. The load_vertex_hashtable and load_edge_hashtable functions have many common chunks. Would it be a good idea to refactor these two functions to separate the common chunks into a new function?

@rafsun-yu Yes, please look into refactoring that code if it looks like it is possible to condense it.

jrgemignani avatar Oct 17 '22 23:10 jrgemignani

A patch for that bug has been applied to the master branch.

jrgemignani avatar Oct 17 '22 23:10 jrgemignani

@jrgemignani Thank you. Your change passes the new test. I sent the PR #336.

rafsun42 avatar Oct 18 '22 00:10 rafsun42

@jrgemignani I've opened the PR #341. Please let me know if I should make any changes,

PragyanD avatar Oct 26 '22 14:10 PragyanD

Completed this task.

rafsun42 avatar Oct 26 '22 18:10 rafsun42