Issue 7271049: code review 7271049: snappy-go: Add snappy C++ implementation benchmarks. (Closed)
|
Created:
11 years, 10 months ago by M-A Modified:
11 years, 9 months ago Reviewers:
CC:
nigeltao, minux1, golang-dev Visibility:
Public. |
Descriptionsnappy-go: Add snappy C++ implementation benchmarks.
The benchmark test data files are automatically downloaded from the
snappy subversion server and used for benchmark with the command:
go test -bench=Benchmark_.* -download
UValidate benchmarks are not supported since snappy-go doesn't support
validating a packet without decompressing it.
Patch Set 1 #Patch Set 2 : diff -r e70163b7ede0 https://code.google.com/p/snappy-go #
Total comments: 32
Patch Set 3 : diff -r e70163b7ede0 https://code.google.com/p/snappy-go #Patch Set 4 : diff -r e70163b7ede0 https://code.google.com/p/snappy-go #
Total comments: 10
Patch Set 5 : diff -r e70163b7ede0 https://code.google.com/p/snappy-go #
Total comments: 3
MessagesTotal messages: 11
https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go File snappy/snappy_test.go (right): https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:132: // Note: the file is OS-language dependent so the resulting values are not For example in my system this file contains French words. I just added a warning but I'm not fixing this "potential problem".
Sign in to reply to this message.
https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go File snappy/snappy_test.go (right): https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcode21 snappy/snappy_test.go:21: var flagDownload = flag.Bool("download", false, "If true, download any missing files before running benchmarks") s/flagDownload/download/ https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcode94 snappy/snappy_test.go:94: b.StartTimer() If you make this b.ResetTimer(), you won't need the "This function assumes the timer was stopped already" comment above. I don't think ResetTimer was in the API when this test was originally written. https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:110: func readContent(b *testing.B, filepath string) []byte { I would s/readContent/readFile/, and s/filepath/filename/ would be consistent with ioutil.ReadFile. https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:113: b.Fatalf("Failed reading %s: %s", filepath, err) s/Failed/failed/ and similarly below. Go error messages typically do not form complete sentences, as they're often prefixed by more context. https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:121: func expand(src []byte, n int) []byte { Add a doc comment above: // expand returns a slice of length n containing copies of src. https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:132: // Note: the file is OS-language dependent so the resulting values are not On 2013/02/04 19:06:33, M-A wrote: > For example in my system this file contains French words. I just added a warning > but I'm not fixing this "potential problem". Ack. https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:151: var testData = []struct { s/testData/testFiles/ and s/benchData/benchFile/ ? https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:155: // The label are not used but the data is kept as-is so it can be copy pasted Move this a few lines up: // testData's values are copied directly from // https://code.google.com/p/snappy/source/browse/trunk/snappy_unittest.cc // The label field is unused in snappy-go. var testData = []struct { https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:178: // testDataExists ensures all test data files are present in $PWD/testdata/. "all" isn't right since it takes (n int) as arguments, but I'd just inline this function where it's called (at line 222). https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:180: fileName := filepath.Join("testdata", testData[n].filename) s/fileName/filename/ and similarly below is consistent with the standard library. https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:187: // downloadSnappyTestdata downloads the official snappy C implementation s/C/C++/ https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:189: func downloadSnappyTestdata() error { s/Snappy//, since we're already in package snappy. https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:190: baseUrl := "https://snappy.googlecode.com/svn/trunk/testdata/" s/baseUrl/baseURL/ https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:194: for n := range testData { Alternatively: for _, td := range testData { etc(td.filename) } https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:195: fileName := filepath.Join("testdata", testData[n].filename) It might work better if the code inside this loop here was its own function (with a named return), and used defers instead of manual clean-ups: func downloadTestdata(n int) (errRet error) { const baseURL = "https://etc/" filename := filepath.Join("testdata", testData[n].filename) f, err := os.Create(filename) if err != nil { return fmt.Errorf(etc, err) } defer f.Close() defer func(){ if errRet != nil { os.Remove(filename) } }() resp, err := http.Get(baseURL + testData[n].filename) if err != nil { return fmt.Errorf(etc, err) } defer resp.Body.Close() _, err = io.Copy(f, resp.Body) if err != nil { return fmt.Errorf(etc, err) } return nil } https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:238: // Naming convention is kept similar to what snappy's C implementation uses. C++.
Sign in to reply to this message.
please also add this patch: diff -r e70163b7ede0 .hgignore --- a/.hgignore Wed Jun 06 19:42:12 2012 +1000 +++ b/.hgignore Fri Feb 08 03:19:12 2013 +0800 @@ -1,4 +1,5 @@ syntax:glob +snappy/testdata .DS_Store .git .gitignore
Sign in to reply to this message.
Hello nigeltao@golang.org, minux.ma@gmail.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/snappy-go
Sign in to reply to this message.
[Newbie here, is there a way to upload without sending an email?] Also added .hgignore but I noticed it's really full of unrelated stuff. https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go File snappy/snappy_test.go (right): https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcode21 snappy/snappy_test.go:21: var flagDownload = flag.Bool("download", false, "If true, download any missing files before running benchmarks") On 2013/02/05 03:46:20, nigeltao wrote: > s/flagDownload/download/ Done. https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcode94 snappy/snappy_test.go:94: b.StartTimer() On 2013/02/05 03:46:20, nigeltao wrote: > If you make this b.ResetTimer(), you won't need the "This function assumes the > timer was stopped already" comment above. I don't think ResetTimer was in the > API when this test was originally written. Thanks, I didn't realize about this method. https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:110: func readContent(b *testing.B, filepath string) []byte { On 2013/02/05 03:46:20, nigeltao wrote: > I would s/readContent/readFile/, and s/filepath/filename/ would be consistent > with ioutil.ReadFile. Done. https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:113: b.Fatalf("Failed reading %s: %s", filepath, err) On 2013/02/05 03:46:20, nigeltao wrote: > s/Failed/failed/ and similarly below. Go error messages typically do not form > complete sentences, as they're often prefixed by more context. Done. https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:121: func expand(src []byte, n int) []byte { On 2013/02/05 03:46:20, nigeltao wrote: > Add a doc comment above: > > // expand returns a slice of length n containing copies of src. Done. https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:151: var testData = []struct { On 2013/02/05 03:46:20, nigeltao wrote: > s/testData/testFiles/ and s/benchData/benchFile/ ? Done. https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:155: // The label are not used but the data is kept as-is so it can be copy pasted On 2013/02/05 03:46:20, nigeltao wrote: > Move this a few lines up: > > // testData's values are copied directly from > // https://code.google.com/p/snappy/source/browse/trunk/snappy_unittest.cc > // The label field is unused in snappy-go. > var testData = []struct { Done. https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:178: // testDataExists ensures all test data files are present in $PWD/testdata/. On 2013/02/05 03:46:20, nigeltao wrote: > "all" isn't right since it takes (n int) as arguments, but I'd just inline this > function where it's called (at line 222). Done. https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:180: fileName := filepath.Join("testdata", testData[n].filename) On 2013/02/05 03:46:20, nigeltao wrote: > s/fileName/filename/ and similarly below is consistent with the standard > library. Done. https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:187: // downloadSnappyTestdata downloads the official snappy C implementation On 2013/02/05 03:46:20, nigeltao wrote: > s/C/C++/ Done. https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:189: func downloadSnappyTestdata() error { On 2013/02/05 03:46:20, nigeltao wrote: > s/Snappy//, since we're already in package snappy. Done. https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:190: baseUrl := "https://snappy.googlecode.com/svn/trunk/testdata/" On 2013/02/05 03:46:20, nigeltao wrote: > s/baseUrl/baseURL/ Done. https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:194: for n := range testData { On 2013/02/05 03:46:20, nigeltao wrote: > Alternatively: > for _, td := range testData { > etc(td.filename) > } Done. https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:195: fileName := filepath.Join("testdata", testData[n].filename) On 2013/02/05 03:46:20, nigeltao wrote: > It might work better if the code inside this loop here was its own function > (with a named return), and used defers instead of manual clean-ups: I hadn't realized yet you could read the return value from a deferred function. That's awesome, thanks! Done but passed basename instead of the index. https://codereview.appspot.com/7271049/diff/2001/snappy/snappy_test.go#newcod... snappy/snappy_test.go:238: // Naming convention is kept similar to what snappy's C implementation uses. On 2013/02/05 03:46:20, nigeltao wrote: > C++. Done.
Sign in to reply to this message.
On Friday, February 8, 2013, maruel wrote: > [Newbie here, is there a way to upload without sending an email?] use "hg upload 7271049" > > Also added .hgignore but I noticed it's really full of unrelated stuff. > use "hg file 7271049 .hgignore" to include that file in this cl, and re-"hg upload 7271049".
Sign in to reply to this message.
On 2013/02/07 21:10:39, minux wrote: > use "hg file 7271049 .hgignore" to include that file in this cl, > and re-"hg upload 7271049". Thanks, updated.
Sign in to reply to this message.
https://codereview.appspot.com/7271049/diff/13001/snappy/snappy_test.go File snappy/snappy_test.go (right): https://codereview.appspot.com/7271049/diff/13001/snappy/snappy_test.go#newco... snappy/snappy_test.go:88: l, err := DecodedLen(src) I would s/l/n/, but isn't l just len(src), where src is the shadowed func argument and not the encoded bytes? Maybe the src, err := Encode(nil, src) should instead be encoded, err := Encode(nil, src) https://codereview.appspot.com/7271049/diff/13001/snappy/snappy_test.go#newco... snappy/snappy_test.go:123: n := copy(x, src) I think that s/n/i/ would be clearer. https://codereview.appspot.com/7271049/diff/13001/snappy/snappy_test.go#newco... snappy/snappy_test.go:176: // The test data files are present at this canonical url. s/url/URL/ https://codereview.appspot.com/7271049/diff/13001/snappy/snappy_test.go#newco... snappy/snappy_test.go:203: // downloadTestdata downloads the official snappy C++ implementation Missing final 's' on "downloadTestdatas", but I'd just inline it into benchFile: if !*download { b.Fatal("test data not found; skipping benchmark without the -download flag") } // Download the official snappy C++ implementation reference // test data files for benchmarking. if err := os.Mkdir("testdata", 0777); err != nil && !os.IsExist(err) { b.Fatalf("failed to create testdata: %s", err) } for _, tf := range testFiles { etc } https://codereview.appspot.com/7271049/diff/13001/snappy/snappy_test.go#newco... snappy/snappy_test.go:209: for _, td := range testFiles { s/td/tf/
Sign in to reply to this message.
https://codereview.appspot.com/7271049/diff/13001/snappy/snappy_test.go File snappy/snappy_test.go (right): https://codereview.appspot.com/7271049/diff/13001/snappy/snappy_test.go#newco... snappy/snappy_test.go:88: l, err := DecodedLen(src) On 2013/02/10 01:09:06, nigeltao wrote: > I would s/l/n/, but isn't l just len(src), where src is the shadowed func > argument and not the encoded bytes? Maybe the > src, err := Encode(nil, src) > should instead be > encoded, err := Encode(nil, src) Done. https://codereview.appspot.com/7271049/diff/13001/snappy/snappy_test.go#newco... snappy/snappy_test.go:123: n := copy(x, src) On 2013/02/10 01:09:06, nigeltao wrote: > I think that s/n/i/ would be clearer. Done. https://codereview.appspot.com/7271049/diff/13001/snappy/snappy_test.go#newco... snappy/snappy_test.go:176: // The test data files are present at this canonical url. On 2013/02/10 01:09:06, nigeltao wrote: > s/url/URL/ Done. https://codereview.appspot.com/7271049/diff/13001/snappy/snappy_test.go#newco... snappy/snappy_test.go:203: // downloadTestdata downloads the official snappy C++ implementation On 2013/02/10 01:09:06, nigeltao wrote: > Missing final 's' on "downloadTestdatas", but I'd just inline it into benchFile: > > if !*download { > b.Fatal("test data not found; skipping benchmark without > the -download flag") > } > // Download the official snappy C++ implementation reference > // test data files for benchmarking. > if err := os.Mkdir("testdata", 0777); err != nil && !os.IsExist(err) { > b.Fatalf("failed to create testdata: %s", err) > } > for _, tf := range testFiles { > etc > } Done. https://codereview.appspot.com/7271049/diff/13001/snappy/snappy_test.go#newco... snappy/snappy_test.go:209: for _, td := range testFiles { On 2013/02/10 01:09:06, nigeltao wrote: > s/td/tf/ Done. https://codereview.appspot.com/7271049/diff/16002/snappy/snappy_test.go File snappy/snappy_test.go (left): https://codereview.appspot.com/7271049/diff/16002/snappy/snappy_test.go#oldco... snappy/snappy_test.go:99: b.SetBytes(int64(len(src))) I did a significant change to match how snappy C++ implementation measure performance. https://code.google.com/p/snappy/source/browse/trunk/snappy_unittest.cc#1068 It measures in uncompressed bytes for UFlat tests so I changed it here to match.
Sign in to reply to this message.
LGTM, submitting... https://codereview.appspot.com/7271049/diff/16002/snappy/snappy_test.go File snappy/snappy_test.go (right): https://codereview.appspot.com/7271049/diff/16002/snappy/snappy_test.go#newco... snappy/snappy_test.go:87: // bandwidth is in amount of uncompressed data. s/band/Band/ https://codereview.appspot.com/7271049/diff/16002/snappy/snappy_test.go#newco... snappy/snappy_test.go:146: // testFiles's values are copied directly from Drop the s after the '.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/snappy-go/source/detail?r=f20185d4bf23 *** snappy-go: Add snappy C++ implementation benchmarks. The benchmark test data files are automatically downloaded from the snappy subversion server and used for benchmark with the command: go test -bench=Benchmark_.* -download UValidate benchmarks are not supported since snappy-go doesn't support validating a packet without decompressing it. R=nigeltao, minux.ma CC=golang-dev https://codereview.appspot.com/7271049 Committer: Nigel Tao <nigeltao@golang.org>
Sign in to reply to this message.
|
