Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize DebugNode flatten_to #7386

Closed
mattsse opened this issue Mar 12, 2024 · 10 comments
Closed

Optimize DebugNode flatten_to #7386

mattsse opened this issue Mar 12, 2024 · 10 comments
Labels
first issue A good way to start contributing T-feature Type: feature

Comments

@mattsse
Copy link
Member

mattsse commented Mar 12, 2024

Component

Forge

Describe the feature you would like

ref #7382

This currently does a bunch of expensive clones:

/// Extends the debug arena.
#[inline]
pub fn debug_arena(mut self, arena: &DebugArena) -> Self {
arena.flatten_to(0, &mut self.debug_arena);
self
}

/// Recursively traverses the tree of debug nodes and flattens it into the given list.
///
/// See [`flatten`](Self::flatten) for more information.
pub fn flatten_to(&self, entry: usize, out: &mut Vec<DebugNodeFlat>) {
let node = &self.arena[entry];
if !node.steps.is_empty() {
out.push(node.flat());
}
for child in &node.children {
self.flatten_to(*child, out);
}
}

since we're consuming the type, we can get rid of the clones by rewriting this with a stack approach

Additional context

image
@mattsse mattsse added first issue A good way to start contributing T-feature Type: feature labels Mar 12, 2024
@qiweiii
Copy link
Contributor

qiweiii commented Mar 13, 2024

May I know how do you generate the image under Additional context

@PanGan21
Copy link
Contributor

Hi @mattsse .
I am interested to pick this up. Would a VecDeque be a good option for our use case?

@mattsse
Copy link
Member Author

mattsse commented Mar 13, 2024

Hi @mattsse . I am interested to pick this up. Would a VecDeque be a good option for our use case?

yeah, I think it should be similar to

https://github.com/paradigmxyz/evm-inspectors/blob/d7317c7fbf38d5e5df2393beee0414c987ea440a/src/tracing/builder/geth.rs#L46

@mattsse
Copy link
Member Author

mattsse commented Mar 13, 2024

May I know how do you generate the image under Additional context

this is https://github.com/mstange/samply

@PanGan21
Copy link
Contributor

Hi @mattsse . I am interested to pick this up. Would a VecDeque be a good option for our use case?

this is https://github.com/mstange/samply

I will give it a try then!

@MuhtasimTanmoy
Copy link

MuhtasimTanmoy commented Mar 13, 2024

Would like to give it a try as well.

Does this change looks ok?

pub fn flatten_to(&self, entry: usize, out: &mut Vec<DebugNodeFlat>) {

      let mut stack = VecDeque::new();
      stack.push_back(entry);

      while let Some(entry) = stack.pop_back() {
          let node = &self.arena[entry];

          if !node.steps.is_empty() {
              out.push(node.flat());
          }

          for child in node.children.iter().rev() {
              stack.push_back(*child);
          }
      }
 }

Also, what needs to be done to test it?

@PanGan21
Copy link
Contributor

PanGan21 commented Mar 13, 2024

Sure, no problem. Let me know if I can still help with something

@PanGan21 PanGan21 removed their assignment Mar 13, 2024
@MuhtasimTanmoy
Copy link

@mattsse What command should generate the profiler image from additional context? samply record ..... ?
How to measure improvement?

As suggested by @DaniPopes how should we work on this function and try to get rid this clone?

/// Flattens this node into a [`DebugNodeFlat`].
pub fn flat(&self) -> DebugNodeFlat {
    DebugNodeFlat { address: self.address, kind: self.kind, steps: self.steps.clone() }
}

@lakshya-sky
Copy link
Contributor

IMO we can't get rid of clones since we are passing DebugArena by ref, but somebody can correct me on this.

@DaniPopes
Copy link
Member

You're right, we have to pass by reference due to the nested data structure. I don't think this is worth it.

@DaniPopes DaniPopes closed this as not planned Won't fix, can't repro, duplicate, stale Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
first issue A good way to start contributing T-feature Type: feature
Projects
None yet
Development

No branches or pull requests

6 participants