rizin icon indicating copy to clipboard operation
rizin copied to clipboard

Refactor rizin graph in rz_util

Open Heersin opened this issue 1 year ago • 4 comments

Your checklist for this pull request

  • [ ] I've read the guidelines for contributing to this repository
  • [ ] I made sure to follow the project's coding style
  • [ ] I've documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
  • [ ] I've added tests that prove my fix is effective or that my feature works (if possible)
  • [ ] I've updated the rizin book with the relevant information (if needed)

Detailed description

working on implement functions Add more graph representation for rizin, see https://github.com/rizinorg/rizin/issues/4231

  1. support identifier for nodes
  2. matrix based graph

Test plan

  1. unit test
  2. replace old graph api and test

Closing issues

...

Heersin avatar Dec 22 '24 16:12 Heersin

Looks already way way better then before! Thanks a lot.

Some questions you might have already considered.

1. Should the graph's API mostly work on Node pointers or the node ids? Having both would be nice. If the node content doesn't matter to the user. But working on pointers is fine for me as well for now.

2. Runtime complexity for getting neighbors. Please let's not clone the nodes every time the neighbor list is requested. This doesn't scale well. Instead a read only `RzIterator` over the lists/matrix is a better fit I think.

3. Could the elements in the List based graph implementation contain edges? It would maybe reduce internal complexity getting the neighbors. Especially for directed graphs.
  1. yes, this is also something I have been contemplating. don't know if it would be confused to provide both way to access. so I defined them mostly work on Node pointer, and they could turn id into pointer through hash_find
  2. exactly, it did bring crazy workload for runtime. I'm looking for a better way to return neighbors, do u know which file provide a best practice sample for returning RzIterator?
  3. no they don't contain explicit edges, initially I implemented it as edge and nodes, but later remove edges to abstract the RzGraph and implement PoC. You r right It's good to have edge in impl for list-based graph, and to reduce the complexity, thanks for point out that

Heersin avatar Jan 09 '25 11:01 Heersin

do u know which file provide a best practice sample for returning RzIterator

You can checkout ht_inc.c. The functions Ht_(as_iter_keys), Ht_(as_iter_mut) and the corresponding (iter_next etc.) return an iterator over a hash map. Your usecase is even simpler I think.

no they don't contain explicit edges, initially I implemented it as edge and nodes, but later remove edges to abstract the RzGraph and implement PoC. You r right It's good to have edge in impl for list-based graph, and to reduce the complexity, thanks for point out that

You make a good point there. Considering code complexity (meaning: many lines of code) vs. low runtime complexity.

If you don't want to add an edge list for now (because too much work/ too many things to consider), it is fine. We can still extend it.

cc @wargio

Rot127 avatar Jan 09 '25 13:01 Rot127

agreed with @Rot127

I think it is ok to have first an implementation that works, then we can improve the execution time.

wargio avatar Jan 09 '25 13:01 wargio

@Heersin have you had any updates since?

notxvilka avatar Nov 14 '25 16:11 notxvilka