Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 3 Aug 2023 10:35:59 -0300
From:   Arnaldo Carvalho de Melo <acme@kernel.org>
To:     Namhyung Kim <namhyung@kernel.org>
Cc:     Jiri Olsa <jolsa@kernel.org>, Ian Rogers <irogers@google.com>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-perf-users@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH 2/2] perf hists browser: Fix the number of entries for
 'e' key
Message-ID: <ZMutP7K0SewD7M9x@kernel.org>
References: <20230731094934.1616495-1-namhyung@kernel.org>
 <20230731094934.1616495-2-namhyung@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230731094934.1616495-2-namhyung@kernel.org>
Precedence: bulk

Em Mon, Jul 31, 2023 at 02:49:33AM -0700, Namhyung Kim escreveu:
> The 'e' key is to toglle expand/collapse the selected entry only.  But
> the current code has a bug that it only increases the number of entries
> by 1 in the hierarchy mode so users cannot move under the current entry
> after the key stroke.  This is due to a wrong assumption in the
> hist_entry__set_folding().
> 
> The commit b33f922651011 ("perf hists browser: Put hist_entry folding
> logic into single function") factored out the code, but actually it
> should be handled separately.  The hist_browser__set_folding() is to
> update fold state for each entry so it needs to traverse all (child)
> entries regardless of the current fold state.  So it increases the
> number of entries by 1.
> 
> But the hist_entry__set_folding() only cares the currently selected
> entry and its all children.  So it should count all unfolded child
> entries.  This code is implemented in hist_browser__toggle_fold()
> already so we can just call it.

Thanks, tested and applied.

- Arnaldo

 
> Fixes: b33f922651011 ("perf hists browser: Put hist_entry folding logic into single function")
> Cc: stable@vger.kernel.org
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/ui/browsers/hists.c | 58 ++++++++++++++--------------------
>  1 file changed, 24 insertions(+), 34 deletions(-)
> 
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index d8b88f10a48d..70db5a717905 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -407,11 +407,6 @@ static bool hist_browser__selection_has_children(struct hist_browser *browser)
>  	return container_of(ms, struct callchain_list, ms)->has_children;
>  }
>  
> -static bool hist_browser__he_selection_unfolded(struct hist_browser *browser)
> -{
> -	return browser->he_selection ? browser->he_selection->unfolded : false;
> -}
> -
>  static bool hist_browser__selection_unfolded(struct hist_browser *browser)
>  {
>  	struct hist_entry *he = browser->he_selection;
> @@ -584,8 +579,8 @@ static int hierarchy_set_folding(struct hist_browser *hb, struct hist_entry *he,
>  	return n;
>  }
>  
> -static void __hist_entry__set_folding(struct hist_entry *he,
> -				      struct hist_browser *hb, bool unfold)
> +static void hist_entry__set_folding(struct hist_entry *he,
> +				    struct hist_browser *hb, bool unfold)
>  {
>  	hist_entry__init_have_children(he);
>  	he->unfolded = unfold ? he->has_children : false;
> @@ -603,34 +598,12 @@ static void __hist_entry__set_folding(struct hist_entry *he,
>  		he->nr_rows = 0;
>  }
>  
> -static void hist_entry__set_folding(struct hist_entry *he,
> -				    struct hist_browser *browser, bool unfold)
> -{
> -	double percent;
> -
> -	percent = hist_entry__get_percent_limit(he);
> -	if (he->filtered || percent < browser->min_pcnt)
> -		return;
> -
> -	__hist_entry__set_folding(he, browser, unfold);
> -
> -	if (!he->depth || unfold)
> -		browser->nr_hierarchy_entries++;
> -	if (he->leaf)
> -		browser->nr_callchain_rows += he->nr_rows;
> -	else if (unfold && !hist_entry__has_hierarchy_children(he, browser->min_pcnt)) {
> -		browser->nr_hierarchy_entries++;
> -		he->has_no_entry = true;
> -		he->nr_rows = 1;
> -	} else
> -		he->has_no_entry = false;
> -}
> -
>  static void
>  __hist_browser__set_folding(struct hist_browser *browser, bool unfold)
>  {
>  	struct rb_node *nd;
>  	struct hist_entry *he;
> +	double percent;
>  
>  	nd = rb_first_cached(&browser->hists->entries);
>  	while (nd) {
> @@ -640,6 +613,21 @@ __hist_browser__set_folding(struct hist_browser *browser, bool unfold)
>  		nd = __rb_hierarchy_next(nd, HMD_FORCE_CHILD);
>  
>  		hist_entry__set_folding(he, browser, unfold);
> +
> +		percent = hist_entry__get_percent_limit(he);
> +		if (he->filtered || percent < browser->min_pcnt)
> +			continue;
> +
> +		if (!he->depth || unfold)
> +			browser->nr_hierarchy_entries++;
> +		if (he->leaf)
> +			browser->nr_callchain_rows += he->nr_rows;
> +		else if (unfold && !hist_entry__has_hierarchy_children(he, browser->min_pcnt)) {
> +			browser->nr_hierarchy_entries++;
> +			he->has_no_entry = true;
> +			he->nr_rows = 1;
> +		} else
> +			he->has_no_entry = false;
>  	}
>  }
>  
> @@ -659,8 +647,10 @@ static void hist_browser__set_folding_selected(struct hist_browser *browser, boo
>  	if (!browser->he_selection)
>  		return;
>  
> -	hist_entry__set_folding(browser->he_selection, browser, unfold);
> -	browser->b.nr_entries = hist_browser__nr_entries(browser);
> +	if (unfold == browser->he_selection->unfolded)
> +		return;
> +
> +	hist_browser__toggle_fold(browser);
>  }
>  
>  static void ui_browser__warn_lost_events(struct ui_browser *browser)
> @@ -732,8 +722,8 @@ static int hist_browser__handle_hotkey(struct hist_browser *browser, bool warn_l
>  		hist_browser__set_folding(browser, true);
>  		break;
>  	case 'e':
> -		/* Expand the selected entry. */
> -		hist_browser__set_folding_selected(browser, !hist_browser__he_selection_unfolded(browser));
> +		/* Toggle expand/collapse the selected entry. */
> +		hist_browser__toggle_fold(browser);
>  		break;
>  	case 'H':
>  		browser->show_headers = !browser->show_headers;
> -- 
> 2.41.0.487.g6d72f3e995-goog
> 

-- 

- Arnaldo