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

Printing dominator subtrees for functions #51

Merged
merged 3 commits into from
May 11, 2018

Conversation

data-pup
Copy link
Member

This commit aims to fix issue #15. Subtrees of the dominators tree can now be printed, if the --function flag is given to the dominator sub-command. This also works when emitting JSON! This commit also adds a test case for JSON output of the overall dominators tree.

It may be worth noting that if the function name is not found in the dominators tree, then the meta root is used, following the default behavior. I also was not quite sure what to call this flag, we can totally change the name of it if you would like :) Let me know if there are any issues you spot!

@data-pup
Copy link
Member Author

data-pup commented May 10, 2018

Ahh. I believe I know how to fix this. Shouldn't take too long, I don't think.

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

I just have a few nitpicks below, but this is pretty much good to merge :)

Thanks @data-pup !


/// The name of the function whose dominator subtree should be printed.
#[structopt(long = "function", default_value = "")]
func_name: String,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be an optional positional argument, ie twiggy dominators input.wasm my_function? Unsure if that will be easy to do in structopt or not, but if we can do it without too much hassle, then I think that would be a little nicer ergonomics for users.

I'd also name this "subtree" rather than "func_name".

@@ -358,8 +363,25 @@ pub fn dominators(
items.compute_dominator_tree();
items.compute_retained_sizes();

/// Get the Id of the item with the given name.
fn get_item_id(items: &mut ir::Items, name: &str) -> Option<ir::Id> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a generally useful utility method to add to ir::Items:

impl Items {
    ...

    fn get_item_by_name(&self) -> Option<&Item> {
        ...
    }
}

Then, if we just need the id, we can map over the returned option like items.get_item_by_name("blah").map(|item| item.id()).

@fitzgen
Copy link
Member

fitzgen commented May 11, 2018

Oh! And after this merges, would you mind filing a follow up issue to allow the subtree name to be a regex, similar to #58 ? Thanks!

@data-pup
Copy link
Member Author

Sure thing! I can take care of those fixes, shouldn't take too long. Once that's done and this has landed, I'll file that issue, wouldn't mind taking a swing at that too :D

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@fitzgen fitzgen merged commit 0d51746 into rustwasm:master May 11, 2018
@data-pup data-pup deleted the display-dom-subtree branch August 20, 2018 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants