packfile/decoder: speed up packfile iterator when specific type #200

ajnavarro · 2017-01-04T15:00:05Z

If specified, the packfile decoder only decode objects of a specific type, improving the decoding time in this cases.

Added public constructor, NewPackfileIter, to be able to iterate objects of a specific type into a packfile in a low level way.
Modified the Decoder to be able to read object headers first, to only decode specific objects.

If specified, the packfile decoder only decode objects of a specific type, improving the decoding time in this cases. - Added public constructor NewPackfileIter to be able to iterate objects of a specific type into a packfile in a low level way. - Modified the Decoder to be able to read object headers first, to only decode specific objects.

codecov-io · 2017-01-04T15:10:31Z

Current coverage is 76.22% (diff: 87.93%)

Merging #200 into master will decrease coverage by 0.49%

@@             master       #200   diff @@
==========================================
  Files            96         96          
  Lines          6224       6270    +46   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           4775       4779     +4   
- Misses          923        971    +48   
+ Partials        526        520     -6

Powered by Codecov. Last update 133b97e...874330e

smola · 2017-01-04T16:02:46Z

@ajnavarro Please, use the appropiate title for the commit and PR (first package, then message, lowercase).

smola · 2017-01-04T16:03:36Z

plumbing/format/packfile/decoder.go

+	return NewDecoderForType(s, o, plumbing.AnyObject)
+}
+
+func NewDecoderForType(s *Scanner, o storer.EncodedObjectStorer,


add documentation.

smola · 2017-01-04T16:04:45Z

plumbing/format/packfile/decoder.go

+	var realType plumbing.ObjectType
+	switch {
+	case h.Type == plumbing.OFSDeltaObject:
+		realType = d.offsetToType[h.OffsetReference]


offset reference might not be present, check it and do error handling

smola · 2017-01-04T16:04:52Z

plumbing/format/packfile/decoder.go

+	case h.Type == plumbing.OFSDeltaObject:
+		realType = d.offsetToType[h.OffsetReference]
+	case h.Type == plumbing.REFDeltaObject:
+		ofs := d.hashToOffset[h.Reference]


reference might not be present, check it and do error handling

smola · 2017-01-04T16:05:26Z

plumbing/format/packfile/decoder.go

+	}
+
+	var realType plumbing.ObjectType
+	switch {


change this with switch h.Type, then you can use the values directly in cases.

smola · 2017-01-04T16:06:49Z

plumbing/format/packfile/decoder.go

+		return d.decodeByHeader(h)
+	}
+
+	return nil, nil


return nil?
Nope, return the next object with a relevant type.

oh, I see that logic is in the PackfileIter, ok.

smola · 2017-01-04T16:07:28Z

plumbing/format/packfile/decoder.go

 	case plumbing.REFDeltaObject:
 		crc, err = d.fillREFDeltaObjectContent(obj, h.Reference)
 	case plumbing.OFSDeltaObject:
 		crc, err = d.fillOFSDeltaObjectContent(obj, h.OffsetReference)
+	case plumbing.CommitObject, plumbing.TreeObject, plumbing.BlobObject, plumbing.TagObject:


no need to change the order of this

smola · 2017-01-04T16:08:53Z

plumbing/format/packfile/decoder_test.go

+		scanner := packfile.NewScanner(f.Packfile())
+		storage := memory.NewStorage()
+
+		d, err := packfile.NewDecoderForType(scanner, storage, plumbing.CommitObject)


Only CommitObject? Test all types.

smola · 2017-01-04T16:10:13Z

storage/filesystem/object_test.go

@@ -91,3 +91,24 @@ func (s *FsSuite) TestIterWithType(c *C) {
 		c.Assert(err, IsNil)
 	})
 }
+
+func (s *FsSuite) TestPackFileIterator(c *C) {


TestPackFileIterator -> TestPackfileIter (same spelling)

alcortesm · 2017-01-05T08:53:26Z

plumbing/format/packfile/decoder.go

+}
+
+func NewDecoderForType(s *Scanner, o storer.EncodedObjectStorer,
+	t plumbing.ObjectType) (*Decoder, error) {


first thing to do is probably returning an error if t is plumbing.InvalidObject.

In case t is plumbing.OFSDeltaObject or plumbing.REFDeltaObject I would recommend returning also an error.

alcortesm · 2017-01-05T08:55:22Z

plumbing/format/packfile/decoder.go

@@ -174,24 +185,54 @@ func (d *Decoder) decodeObjectsWithObjectStorerTx(count int) error {

 // DecodeObject reads the next object from the scanner and returns it. This
 // method can be used in replacement of the Decode method, to work in a
-// interative way
+// interactive way. If you created a new decoder instance using NewDecoderForType
+// constructor, if the object decoded is not equals to the specified one, null will


s/null/nil/

Why return nil instead of an error or just skip til the next object of that type? The caller wants an object or an error, not something that is not an object, nor an error. Let's keep things easy for the caller doing the work inside the function, not outside.

To do this in a proper way, we should refactor the Decoder to be an Iterator instead of get the elements count from the Scanner and then call to DecodeObject 'count' times. But this was the previous behavior. Could we do this refactor in another PR?

of course, that will be a nice solution.

alcortesm · 2017-01-05T09:01:48Z

plumbing/format/packfile/decoder.go

+	return nil, nil
+}
+
+func (d *Decoder) nextHeader() (*ObjectHeader, error) {


This method, along with 193-195 is redundant. Refactor all these.

alcortesm · 2017-01-05T09:04:55Z

plumbing/format/packfile/decoder.go

@@ -201,7 +242,7 @@ func (d *Decoder) DecodeObject() (plumbing.EncodedObject, error) {
 	}

 	hash := obj.Hash()
-	d.setOffset(hash, h.Offset)
+	d.setOffsetAndType(hash, h.Offset, obj.Type())


set... is quite a bad name, as it does not transmit intention or usefulness. Can we call it memoize or remember instead?

alcortesm · 2017-01-05T09:09:57Z

storage/filesystem/object.go

-		return iter.Next()
-	}
+		iter.position++
+		if obj != nil {


This is what I meant before when commenting on not giving extra work to our users. This method is unnecasary complex due to DecodeObject returning ambigous results. Please change that and simplify this.

Besides this: this if should negated to avoid extra logic an extra indentation:

if obj == nil { continue }

alcortesm · 2017-01-05T09:18:17Z

storage/filesystem/object.go

-		return iter.Next()
-	}
+			if iter.t != plumbing.AnyObject && iter.t != obj.Type() {
+				return iter.Next()


a recursive call inside a for loop? on a condition that can not happen because the code just added to the file above? WTF.

oops, good catch.

smola · 2017-01-05T11:36:59Z

storage/filesystem/object_test.go

 	})
 }

-func (s *FsSuite) TestPackFileIterator(c *C) {
-	fixtures.ByTag(".git").ByTag("packfile").Test(c, func(f *fixtures.Fixture) {
+func (s *FsSuite) TestPackFileIter(c *C) {


TestPackFileIter -> TestPackfileIter

Typo in api documentation

smola suggested changes Jan 4, 2017

View reviewed changes

ajnavarro changed the title ~~Speed up packfile iterator when specific type~~ packfile/decoder: Speed up packfile iterator when specific type Jan 5, 2017

ajnavarro changed the title ~~packfile/decoder: Speed up packfile iterator when specific type~~ packfile/decoder: speed up packfile iterator when specific type Jan 5, 2017

alcortesm suggested changes Jan 5, 2017

View reviewed changes

Requested changes

7129154

smola suggested changes Jan 5, 2017

View reviewed changes

Requested changes

874330e

smola approved these changes Jan 5, 2017

View reviewed changes

alcortesm approved these changes Jan 9, 2017

View reviewed changes

mcuadros merged commit c5f1056 into src-d:master Jan 12, 2017

ajnavarro deleted the improvement/filesystem-storage branch January 12, 2017 09:08

mcuadros pushed a commit that referenced this pull request Jan 31, 2017

packfile/decoder: speed up packfile iterator when specific type (#200)

f9c7c8c

mcuadros mentioned this pull request Nov 19, 2017

format: packfile fix DecodeObjectAt when Decoder has type #646

Merged

gsalingu-ovhus pushed a commit to gsalingu-ovhus/go-git that referenced this pull request Mar 28, 2019

Merge pull request src-d#200 from carsso/1.2.1

7061d0d

Typo in api documentation

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

packfile/decoder: speed up packfile iterator when specific type #200

packfile/decoder: speed up packfile iterator when specific type #200

ajnavarro commented Jan 4, 2017

codecov-io commented Jan 4, 2017 •

edited

Loading

smola commented Jan 4, 2017

smola Jan 4, 2017

ajnavarro Jan 5, 2017

smola Jan 4, 2017

ajnavarro Jan 5, 2017

smola Jan 4, 2017

ajnavarro Jan 5, 2017

smola Jan 4, 2017

ajnavarro Jan 5, 2017

smola Jan 4, 2017

smola Jan 4, 2017

smola Jan 4, 2017

ajnavarro Jan 5, 2017

smola Jan 4, 2017

ajnavarro Jan 5, 2017

smola Jan 4, 2017

alcortesm Jan 5, 2017

alcortesm Jan 5, 2017

alcortesm Jan 5, 2017

ajnavarro Jan 5, 2017

alcortesm Jan 9, 2017 •

edited

Loading

alcortesm Jan 5, 2017

alcortesm Jan 5, 2017

alcortesm Jan 5, 2017

alcortesm Jan 5, 2017

ajnavarro Jan 5, 2017

smola Jan 5, 2017

ajnavarro Jan 5, 2017

packfile/decoder: speed up packfile iterator when specific type #200

packfile/decoder: speed up packfile iterator when specific type #200

Conversation

ajnavarro commented Jan 4, 2017

codecov-io commented Jan 4, 2017 • edited Loading

Current coverage is 76.22% (diff: 87.93%)

smola commented Jan 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcortesm Jan 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 4, 2017 •

edited

Loading

alcortesm Jan 9, 2017 •

edited

Loading