GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Don't search/list every single repo reference when explicit references are provided by elhmn · Pull Request #112 · github/git-sizer

Conversation

elhmn commented Sep 25, 2023

Copy link

Copy Markdown

Contributor

Now that we can provide explicit roots to git-sizer, we don't want to look for other references than the one provided.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Don't search/list every single repo reference when explicit reference…

9d71973

…s are provided

elhmn requested a review from a team as a code owner

September 25, 2023 13:15

mhagger commented Sep 25, 2023

Copy link

Copy Markdown

Member

It would be a nice optimization not to run git for-each-ref at all in the case that no references are being scanned. That might make a significant difference to the runtime in a repository that has a lot of references if only a small part of the history is being scanned.

But this PR as currently written introduces a couple of rough edges:

In the case that both references and objects were specified, the old code scans both. For example,
```
 git-sizer --tags 'HEAD@{1.week.ago}'
```
scans objects that are reachable from tags and also objects that are reachable from the reflog entry for HEAD from a week ago. I think that the optimization should only be used if an argument was specified AND no tag-related options were specified (which is the behavior documented in the --help output).
When this optimization is triggered, the output still includes the lines
```
 | * References                 |           |                                |
 |   * Count                    |     0     |                                |
```
The count usually lists the total number of references (including those that were not selected). It is misleading to say that this count is zero only because we didn't bother to read the references at all. I think it would be better not to output these lines at all when the optimization is in effect (and analogously for the JSON output).

The textual output is defined in HistorySize.contents(). This function already has some code that adds an item for each reference group (in rgis). It could take an additional bool parameter and based on that include or omit the whole "References" section. It's a little bit awkward because of the way sections are constructed; let me know if you want some ideas for how that could be wired up.

elhmn reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Still run git for-each-ref when explicit roots and reference filter…

721e353

…s are specified

When explicit roots and reference filters are both provided, we want to make
sure that the optimisation don't kick in. As we still want to iterate
over all references in order to apply the filters.

Here is an example with git-sizer being run with explicit roots and reference filters
```
git-sizer main~20^{tree} main~10^{tree} --tags --branches
```

elhmn force-pushed the elhmn-limit-tree-traversal-to-refs-passed-by-arguments branch from 135d97e to 721e353 Compare

September 29, 2023 12:09

elhmn added 2 commits

October 5, 2023 12:42


          Don't print the referecence count when explicit roots are provided

0315cba

We printed out the reference count, even when explicit roots were
provided.
```
git-sizer master~50^{tree} 'HEAD@{1.week.ago}' -v
[TRUNCATED_OUTPUT]
| * References                 |           |                                |
|   * Count                    |   187     |                                |
|     * Ignored                |   187     |                                |
|                              |           |                                |
[TRUNCATED_OUTPUT]
```

This is not very useful, as the reference count depends on the list of references
extracted by running `git for-each-ref`.

This commit changes the output to not print the reference count when
explicit roots and no reference filters are provided to git-sizer


          Don't print reference count for json v1

d407c86

The reference count is still displayed when the -j flag is passed to
git-sizer.
```
git-sizer master~50^{tree} 'HEAD@{1.week.ago}' -v -j
[TRUNCATED_OUTPUT]
    "reference_count": 15,
    "reference_groups": {
        "ignored": 15
    },
[TRUNCATED_OUTPUT]
```

We don't want to print the reference count for json v1, when explicit
root are not provided git-sizer

elhmn force-pushed the elhmn-limit-tree-traversal-to-refs-passed-by-arguments branch from 41cd01a to d407c86 Compare

October 5, 2023 12:20

mhagger reviewed

Dec 13, 2023

View reviewed changes

mhagger left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Aside from the line comments, I noticed that this version of the code emits confusing counts in the "references" progress meter, like:

$ git-sizer HEAD^
[…]
Processing references: 1

I think that this count should be zero, since no references were scanned. But the progress meter is actually counting roots, not references. So this is misleading. (I know it's "just" a progress meter, but I know that I glance at those numbers all the time and rely on them.)

I suggest instead iterating over the references and roots separately, with two separate progress meters, like

diff --git a/sizes/graph.go b/sizes/graph.go
index 0fb1c8a..2fb1b71 100644
--- a/sizes/graph.go
+++ b/sizes/graph.go
@@ -269,14 +269,22 @@ func ScanRepositoryUsingGraph(
                return HistorySize{}, err
        }
 
+       // Process roots in two passes just to make the progress meter
+       // output more useful:
+
        progressMeter.Start("Processing references: %d")
        for _, root := range roots {
-               progressMeter.Inc()
                if refRoot, ok := root.(ReferenceRoot); ok {
+                       progressMeter.Inc()
                        graph.RegisterReference(refRoot.Reference(), refRoot.Groups())
                }
+       }
+       progressMeter.Done()
 
+       progressMeter.Start("Processing roots: %d")
+       for _, root := range roots {
                if root.Walk() {
+                       progressMeter.Inc()
                        graph.pathResolver.RecordName(root.Name(), root.OID())
                }
        }

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

murayamajpeg reacted with laugh emoji

All reactions

😄 1 reaction

Comment thread

git-sizer.go

		for _, arg := range flags.Args() {
		oid, err := repo.ResolveObject(arg)
		// If no reference filters and no explicit roots were provided
		if git.IsNoReferencesFilter(rgb.GetTopLevelGroup().GetFilter()) {

mhagger Dec 13, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Instead of revealing internal details via both RefGroupBuilder.GetTopLevelGroup() and refGroup.GetFilter() plus needing the strange function IsNoReferencesFilter(), how about adding a special-purpose method to RefGroupBuilder, something like the following:

// RefoptsSeen returns `true` iff any reference selection options were
// used. It is only valid after `Finish()` has been called.
func (rgb *RefGroupBuilder) RefoptsSeen() bool {
	return rgb.topLevelGroup.filter != git.NoReferencesFilter
}

git.NoReferencesFilter is a singleton, so I think that a simple comparison like this should suffice.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

git/ref_filter.go

Comment on lines +143 to +147


		func IsNoReferencesFilter(val interface{}) bool {
		_, ok := val.(noReferencesFilter)
		return ok
		}

mhagger Dec 13, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

RefGroupBuilder.RefoptsSeen() could make this could go away.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

internal/refopts/ref_group.go

Comment on lines +33 to +36

		func (rg *refGroup) GetFilter() git.ReferenceFilter {
		return rg.filter
		}

mhagger Dec 13, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

RefGroupBuilder.RefoptsSeen() could make this could go away.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

internal/refopts/ref_group_builder.go

Comment on lines +25 to +28

		func (rgb RefGroupBuilder) GetTopLevelGroup() refGroup {
		return rgb.topLevelGroup
		}

mhagger Dec 13, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

RefGroupBuilder.RefoptsSeen() could make this could go away.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

sizes/output.go

Comment on lines +509 to +522

		var refCountSection section
		if !opts.WithoutReferenceCount {
		refCountSection = S(
		"References",
		I("referenceCount", "Count",
		"The total number of references",
		nil, s.ReferenceCount, metric, "", 25e3),
		S(
		"",
		rgis...,
		),
		)
		}

mhagger Dec 13, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This approach seems to depend on the fact that even though the uninitialized section is inserted into the table, it doesn't output anything. I think that's correct ✔️

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

mhagger commented Dec 14, 2023

Copy link

Copy Markdown

Member

(To be clear, the progress meter was already inaccurate before this PR. It just wasn't so obvious, because the references were being scanned and counted as well.)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Andrei99120 commented Aug 18, 2024

Copy link

Copy Markdown

elhmn-limit-tree-traversal-to-refs-passed-by-arguments

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Reviewers

mhagger

mhagger left review comments

At least 1 approving review is required to merge this pull request.

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.