Finalize TarSum Version 1 w/ refactor #8869

jlhawn · 2014-10-30T21:50:57Z

The current Dev version of TarSum includes hashing of extended
file attributes and omits inclusion of modified time headers.

I refactored the logic around the version differences to make it
more clear that the difference between versions is in how tar
headers are selected and ordered.

TarSum Version 1 is now declared with the new Dev version continuing
to track it.

Docker-DCO-1.1-Signed-off-by: Josh Hawn josh.hawn@docker.com (github: jlhawn)

dmcgowan · 2014-10-30T22:05:57Z

pkg/tarsum/versioning.go

 	Version0 Version = iota
-	// Prefix of "tarsum.dev"
+	Version1
+	// VersionDev is the prefix of "tarsum.dev"


Should be "VersionDev has", also pretty sure these comments are intended to specify what the prefix of each type is so either should be with each version or removed.

jlhawn · 2014-10-30T22:39:02Z

I've run the benchmarks on my machine with both the master version of the package and this branch. It looks like we get some negligible to small improvement in speed and a significant improvement in memory performance which is most noticeable in archives with several files. This is probably due to the changes in how allocations are done when selecting tar headers. Have a look:

Benchmark9kTar                  @ master    @ branch    % delta

    ns/op                       12085       12038       -0.389 %
    MB/s                        762.57      765.55      +0.391 %
    B/op                        14183       14200       +0.120 %
    allocs/op                   22          22          +0.000 %


Benchmark9kTarGzi               @ master    @ branch    % delta

    ns/op                       206875      206610      -0.128 %
    MB/s                        44.55       44.61       +0.135 %
    B/op                        1470279     1470294     +0.001 %
    allocs/op                   74          74          +0.000 %


Benchmark1mbSingleFile          @ master    @ branch    % delta

    ns/op                       6505530     6510376     +0.074 %
    MB/s                        161.18      161.06      -0.074 %
    B/op                        42212       41625       -1.391 %
    allocs/op                   352         348         -1.136 %


Benchmark1mbSingleFileTarG      @ master    @ branch    % delta

    ns/op                       19138067    18679059    -2.398 %
    MB/s                        54.79       56.14       +2.464 %
    B/op                        1906895     1906389     -0.027 %
    allocs/op                   3372        3371        -0.030 %


Benchmark1kFiles                @ master    @ branch    % delta

    ns/op                       24156129    23880668    -1.140 %
    MB/s                        43.41       43.91       +1.151 %
    B/op                        2804772     2186320     -22.05 %
    allocs/op                   75203       71120       -5.429 %


Benchmark1kFilesTarG            @ master    @ branch    % delta

    ns/op                       60346171    59714904    -1.046 %
    MB/s                        17.38       17.56       +1.036 %
    B/op                        11574602    10949201    -5.403 %
    allocs/op                   145456      141310      -2.850 %

jlhawn · 2014-10-30T22:41:40Z

@ewindisch @vbatts @unclejack please review

vbatts · 2014-10-31T17:34:47Z

reading over it, it LGTM. Perhaps, the maneuver to drop the mtime header could be more explicit, but at least you have a comment there ;-)

vbatts · 2014-10-31T17:35:40Z

Also, I've made progress on writing a specification for the TarSum, and should have a PR for it. Hopefully next week.

jlhawn · 2014-10-31T23:44:49Z

updated with fixed variable name. I had called something an encoder when I should have called it a selector.

jlhawn · 2014-11-04T00:25:18Z

ping @ewindisch

unclejack · 2014-11-04T17:44:03Z

@jlhawn Why is mtime being removed?

vbatts · 2014-11-04T18:00:33Z

@unclejack the .wh. files primarily. but really are not needed.

unclejack · 2014-11-04T18:07:32Z

LGTM

dmp42 · 2014-11-04T23:04:49Z

ping @vbatts @ewindisch

vbatts · 2014-11-05T05:17:55Z

pkg/tarsum/versioning.go

-	1: "tarsum.dev",
+	Version0:   "tarsum",
+	Version1:   "tarsum.v1",
+	VersionDev: "tarsum.dev",


👍 Thanks for that. I didn't mean to leave those as just integers. :-)

vbatts · 2014-11-05T05:20:45Z

LGTM
(also, your git commit message says "TurSum" :-)

jlhawn · 2014-11-05T05:37:38Z

gah... will amend now.

The current Dev version of TarSum includes hashing of extended file attributes and omits inclusion of modified time headers. I refactored the logic around the version differences to make it more clear that the difference between versions is in how tar headers are selected and ordered. TarSum Version 1 is now declared with the new Dev version continuing to track it. Docker-DCO-1.1-Signed-off-by: Josh Hawn <josh.hawn@docker.com> (github: jlhawn)

jlhawn · 2014-11-06T18:31:16Z

ping @vbatts @unclejack @jfrazelle @crosbymichael @vieux Please review again due to rebase 👍

vbatts · 2014-11-06T19:36:07Z

@jlhawn did you functionally change anything with the rebase, or just double checking from LGTMs?

jlhawn · 2014-11-06T19:39:54Z

no functional changes, no. Just fixed my commit message, and since PR history shows LGTMs before the commit I just wanted double check from maintainers.

vbatts · 2014-11-06T19:42:52Z

right
LGTM

jlhawn · 2014-11-06T20:01:51Z

I've been trying to ping @ewindisch too. It's odd that his GitHub username doesn't autocomplete so I'm not sure if he's even getting notifications about this. :( If that's the case I'll need some extra LGTMs from the core maintainers I guess.

unclejack · 2014-11-08T19:25:03Z

still LGTM

ewindisch · 2014-11-09T00:26:03Z

@jlhawn I've been at a conference this past week. I'll review first chance.

dmp42 · 2014-11-10T17:53:54Z

ping @crosbymichael @vieux @jfrazelle

jessfraz · 2014-11-10T17:56:44Z

pkg/tarsum/versioning.go

+	}
+}
+
+func v1TarHeaderSelect(h *tar.Header) (orderedHeaders [][2]string) {


not really a fan of version number in a function name, could have probably been an interface, but I understand it might be out of scope for this PR

Well, there is an interface tarHeaderSelector. I made it so that the tarHeaderSelectFunc type implements the interface. It's like http.Handler and http.HandlerFunc. The version numbers are in the func name because I hadn't thought of a better way to differentiate them.

jessfraz · 2014-11-10T17:57:26Z

LGTM

jlhawn · 2014-11-10T18:09:20Z

thanks @ewindisch (hey auto-complete works again!)

vbatts · 2014-11-10T21:30:01Z

pkg/tarsum/versioning.go

+	for k := range h.Xattrs {
+		xAttrKeys = append(xAttrKeys, k)
+	}
+	sort.Strings(xAttrKeys)


hrm. hold the phone. we should add tests around the case sensitivity of the these keys. How that relates to sorting and whether there could be collisions.

Ensuring case size of headers will still be accounted for. moby#8869 (comment) Signed-off-by: Vincent Batts <vbatts@redhat.com>

vbatts · 2014-11-11T13:54:51Z

OK. Just gave myself necessary warm-fuzzies.
This PR is just waiting on a stamp-of-approval from @ewindisch.

jlhawn · 2014-11-13T19:48:16Z

ping @ewindisch

ewindisch · 2014-11-13T20:06:38Z

LGTM. I initially had some issues, but upon review, it was nothing that wasn't already in the existing code.

unclejack · 2014-11-13T23:04:30Z

Are we good to merge? Do we want anyone else to review?

vbatts · 2014-11-13T23:06:06Z

LGTM good to go

jlhawn · 2014-11-13T23:08:39Z

+1

Finalize TarSum Version 1 w/ refactor

Ensuring case size of headers will still be accounted for. moby#8869 (comment) Signed-off-by: Vincent Batts <vbatts@redhat.com>

dmcgowan reviewed Oct 30, 2014
View reviewed changes

jlhawn force-pushed the tarsum_version branch from 195f7fe to 520e3ba Compare October 30, 2014 22:36

jlhawn added the Distribution label Oct 30, 2014

jlhawn force-pushed the tarsum_version branch from 520e3ba to f442293 Compare October 30, 2014 23:09

jlhawn force-pushed the tarsum_version branch from f442293 to 6646961 Compare October 31, 2014 23:44

vbatts reviewed Nov 5, 2014
View reviewed changes

jlhawn force-pushed the tarsum_version branch from 6646961 to a7aa2c8 Compare November 5, 2014 05:39

jessfraz reviewed Nov 10, 2014
View reviewed changes

vbatts reviewed Nov 10, 2014
View reviewed changes

vbatts added a commit to vbatts/moby that referenced this pull request Nov 11, 2014

pkg/tarsum: adding more tests

5cd9b75

Ensuring case size of headers will still be accounted for. moby#8869 (comment) Signed-off-by: Vincent Batts <vbatts@redhat.com>

vbatts mentioned this pull request Nov 11, 2014

pkg/tarsum: adding more tests #9086

Merged

unclejack added a commit that referenced this pull request Nov 13, 2014

Merge pull request #8869 from jlhawn/tarsum_version

3aa2245

Finalize TarSum Version 1 w/ refactor

unclejack merged commit 3aa2245 into moby:master Nov 13, 2014

mapuri pushed a commit to mapuri/docker that referenced this pull request Dec 4, 2014

pkg/tarsum: adding more tests

15e7213

Ensuring case size of headers will still be accounted for. moby#8869 (comment) Signed-off-by: Vincent Batts <vbatts@redhat.com>

vbatts mentioned this pull request Dec 18, 2014

Tarsum insecurity #9719

Closed

jlhawn deleted the tarsum_version branch July 31, 2015 18:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finalize TarSum Version 1 w/ refactor #8869

Finalize TarSum Version 1 w/ refactor #8869

jlhawn commented Oct 30, 2014

dmcgowan Oct 30, 2014

jlhawn commented Oct 30, 2014

jlhawn commented Oct 30, 2014

vbatts commented Oct 31, 2014

vbatts commented Oct 31, 2014

jlhawn commented Oct 31, 2014

jlhawn commented Nov 4, 2014

unclejack commented Nov 4, 2014

vbatts commented Nov 4, 2014

unclejack commented Nov 4, 2014

dmp42 commented Nov 4, 2014

vbatts Nov 5, 2014

vbatts commented Nov 5, 2014

jlhawn commented Nov 5, 2014

jlhawn commented Nov 6, 2014

vbatts commented Nov 6, 2014

jlhawn commented Nov 6, 2014

vbatts commented Nov 6, 2014

jlhawn commented Nov 6, 2014

unclejack commented Nov 8, 2014

ewindisch commented Nov 9, 2014

dmp42 commented Nov 10, 2014

jessfraz Nov 10, 2014

jlhawn Nov 10, 2014

jessfraz commented Nov 10, 2014

jlhawn commented Nov 10, 2014

vbatts Nov 10, 2014

vbatts commented Nov 11, 2014

jlhawn commented Nov 13, 2014

ewindisch commented Nov 13, 2014

unclejack commented Nov 13, 2014

vbatts commented Nov 13, 2014

jlhawn commented Nov 13, 2014

Finalize TarSum Version 1 w/ refactor #8869

Finalize TarSum Version 1 w/ refactor #8869

Conversation

jlhawn commented Oct 30, 2014

dmcgowan Oct 30, 2014

Choose a reason for hiding this comment

jlhawn commented Oct 30, 2014

jlhawn commented Oct 30, 2014

vbatts commented Oct 31, 2014

vbatts commented Oct 31, 2014

jlhawn commented Oct 31, 2014

jlhawn commented Nov 4, 2014

unclejack commented Nov 4, 2014

vbatts commented Nov 4, 2014

unclejack commented Nov 4, 2014

dmp42 commented Nov 4, 2014

vbatts Nov 5, 2014

Choose a reason for hiding this comment

vbatts commented Nov 5, 2014

jlhawn commented Nov 5, 2014

jlhawn commented Nov 6, 2014

vbatts commented Nov 6, 2014

jlhawn commented Nov 6, 2014

vbatts commented Nov 6, 2014

jlhawn commented Nov 6, 2014

unclejack commented Nov 8, 2014

ewindisch commented Nov 9, 2014

dmp42 commented Nov 10, 2014

jessfraz Nov 10, 2014

Choose a reason for hiding this comment

jlhawn Nov 10, 2014

Choose a reason for hiding this comment

jessfraz commented Nov 10, 2014

jlhawn commented Nov 10, 2014

vbatts Nov 10, 2014

Choose a reason for hiding this comment

vbatts commented Nov 11, 2014

jlhawn commented Nov 13, 2014

ewindisch commented Nov 13, 2014

unclejack commented Nov 13, 2014

vbatts commented Nov 13, 2014

jlhawn commented Nov 13, 2014