Re: [EGIT PATCH 4/6] Add tags to the graphical history display.

From: Robin Rosenberg <robin.rosenberg@dewire.com>
Date: Oct 6, 2008, 21:58 UTC
Message-ID: <200810062358.44954.robin.rosenberg@dewire.com>
In-Reply-To: <20081006080834.GC27516@spearce.org>

måndagen den 6 oktober 2008 10.08.34 skrev Shawn O. Pearce:

Show 10 quoted lines

> FYI, my comments don't fully cover this patch yet.
> 
> > -	protected PlotCommit(final AnyObjectId id) {
> > +	protected PlotCommit(final AnyObjectId id, final Ref[] tags) {
> >  		super(id);
> > +		this.refs = tags;
> 
> this.refs isn't final.  There is no reason to be adding it to the
> constructor and having this ripple effect all the way up into the
> RevWalk class.

Eh, it doesn't, ecept I added a methos to RevWalk than can return tags, which it doesn't unless the information is attached.

Show 5 quoted lines

> Maybe PlotWalk should override next() and only set PlotCommit.refs on
> things returned from super.next()?  This way we only do tag lookup
> on commits that the filtering rules have said should be shown in
> to the application, but the refs should still be inserted prior to
> the application seeing the RevCommit.

I missed something here I think. Thanks. Maybe I thought... doesn't matter. I'll rewrite the changes in and related to this completely.

Show 10 quoted lines

> > @@ -54,6 +66,7 @@
> >  	public PlotWalk(final Repository repo) {
> >  		super(repo);
> >  		super.sort(RevSort.TOPO, true);
> > +		reverseRefMap = repo.getAllRefsByPeeledObjectId();
> 
> I wonder if this shouldn't be done as part of the StartGenerator
> (or something like it but specialized for PlotWalk).  I only say
> that because a reused PlotWalk may want to make sure its ref map
> is current with the repository its about to walk against.

noted.

> You are inside of a PlotWalk, which extends RevWalk, which has very
> aggressive caching of RevCommit and RevTag data instances, and very
> fast parsers.  Much faster than the legacy Commit and Tag classes.

Maybe we should rid us of them completely and make new commit from these classes of possible more lightweight ones. The old classes were straightforward but are on overtime now.

Show 7 quoted lines

> I think we should use the RevCommit and RevTag classes to handle
> sorting here.  RevCommit already has the committer time cached
> and stored in an int.  RevCommit probably should learn to do the
> same for its "tagger" field.  The tiny extra bloat (1 word) that
> adds to a RevTag instance is worth it when we consider implementing
> something like this and/or git-describe where sorting tags by their
> dates is useful.

As you noted earlier we usually have at most one ref per commit to sort. Not much to optimize for speed here with current repos. For a one-element list the compare routine wil not even be invoked.

Show 22 quoted lines

> 
> > +			tags = list.toArray(new Ref[list.size()]);
> 
> I wonder if using a Ref[] here even makes sense given that the data
> is stored in a List<Ref>.  I use RevCommit[] inside of RevCommit
> generally because the number of entries in the array is 1 or 2 and
> the array is smaller than say an ArrayList.
> 
> In hindsight those RevCommit[] probably should be a List<RevCommit>
> with different list implementations based on the number of parents
> needed:
> 
> 	0 parents:  Collections.emptyList()
> 	1 parent:   Collections.singletonList()
> 	2 parents:  some especialized AbstractList subclass
> 	3 parents:  ArrayList
> 
> I think it would actually use less memory per instance, which is
> a huge bonus, but we'd pay a downcasting penalty on each access.
> HotSpot is supposed to be really good at removing the downcast
> penalty from say java.util.List, but I don't if it can beat a typed
> array access.

I guess measuring is the right way. For these small arrays, my bet is that List is way faster because we do not need any bounds checking. Hotspot is reasonably good at optimizing those away, but that won't help for much small arrays.

> Sorry I got off on a bit of a tangent here.  I'm just trying to
> point out that the primary reason I've usd an array before is
> probably moot here since the data is already in a List.

Could it be that arrays are often better then lists.

Show 23 quoted lines

> > diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java
> > index d7e4c58..41d57c6 100644
> > --- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java
> > +++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java
> > @@ -53,6 +53,7 @@
> 
> IMHO this class shouldn't need to be modified.
> 
> > @@ -541,7 +542,7 @@ public RevTree lookupTree(final AnyObjectId id) {
> >  	public RevCommit lookupCommit(final AnyObjectId id) {
> >  		RevCommit c = (RevCommit) objects.get(id);
> >  		if (c == null) {
> > -			c = createCommit(id);
> > +			c = createCommit(id, getTags(id));
> 
> This code is performance critical to commit parsing.  Trying to
> lookup tags for every commit we evaluate, especially ones that we
> will never show in the UI (because they are uninteresting) but that
> we still need to parse in order to derive the merge base is just
> a huge waste of time.
> 
> Same applies for the lookupAny and parseAny methods.
>

Ack.

-- robin