hugolib: Compare every element in pages cache
authorBjørn Erik Pedersen <bjorn.erik.pedersen@gmail.com>
Fri, 21 Sep 2018 08:10:11 +0000 (10:10 +0200)
committerBjørn Erik Pedersen <bjorn.erik.pedersen@gmail.com>
Fri, 21 Sep 2018 09:21:15 +0000 (11:21 +0200)
It is slightly slower, but correctnes is, of course, more important:

```bash
benchmark                             old ns/op     new ns/op     delta
BenchmarkSortByWeightAndReverse-4     367           645           +75.75%

benchmark                             old allocs     new allocs     delta
BenchmarkSortByWeightAndReverse-4     2              2              +0.00%

benchmark                             old bytes     new bytes     delta
BenchmarkSortByWeightAndReverse-4     64            64            +0.00%
```

Running the same benchmark without any cache (i.e. resorting the slice on every iteration) and then compare it to the current version shows that it still is plenty worth it:

```bash
▶ benchcmp 2.bench 1.bench
benchmark                             old ns/op     new ns/op     delta
BenchmarkSortByWeightAndReverse-4     1358757       645           -99.95%

benchmark                             old allocs     new allocs     delta
BenchmarkSortByWeightAndReverse-4     17159          2              -99.99%

benchmark                             old bytes     new bytes     delta
BenchmarkSortByWeightAndReverse-4     274573        64            -99.98%
```

Closes #5239

hugolib/pageCache.go
hugolib/pageCache_test.go
hugolib/pageSort_test.go
hugolib/pages_related.go

index 2ac58492002c376c9d1a3f7c36270b6d1fa97314..15598a4f0546d6e9716a3316b5e96e2fc0ee234e 100644 (file)
@@ -27,7 +27,7 @@ func (entry pageCacheEntry) matches(pageLists []Pages) bool {
                return false
        }
        for i, p := range pageLists {
-               if !fastEqualPages(p, entry.in[i]) {
+               if !pagesEqual(p, entry.in[i]) {
                        return false
                }
        }
@@ -103,10 +103,8 @@ func (c *pageCache) getP(key string, apply func(p *Pages), pageLists ...Pages) (
 
 }
 
-// "fast" as in: we do not compare every element for big slices, but that is
-// good enough for our use cases.
-// TODO(bep) there is a similar method in pagination.go. DRY.
-func fastEqualPages(p1, p2 Pages) bool {
+// pagesEqual returns whether p1 and p2 are equal.
+func pagesEqual(p1, p2 Pages) bool {
        if p1 == nil && p2 == nil {
                return true
        }
@@ -123,13 +121,7 @@ func fastEqualPages(p1, p2 Pages) bool {
                return true
        }
 
-       step := 1
-
-       if len(p1) >= 50 {
-               step = len(p1) / 10
-       }
-
-       for i := 0; i < len(p1); i += step {
+       for i := 0; i < len(p1); i++ {
                if p1[i] != p2[i] {
                        return false
                }
index 52a7f449441081b7d65ca2f850476757913ef997..48f595f8690fdf85745a5a341659fa876c02c963 100644 (file)
@@ -57,8 +57,8 @@ func TestPageCache(t *testing.T) {
                                l1.Unlock()
                                p2, c2 := c1.get("k1", nil, p)
                                assert.True(t, c2)
-                               assert.True(t, fastEqualPages(p, p2))
-                               assert.True(t, fastEqualPages(p, pages))
+                               assert.True(t, pagesEqual(p, p2))
+                               assert.True(t, pagesEqual(p, pages))
                                assert.NotNil(t, p)
 
                                l2.Lock()
index 02fa8dcc99e64d56e6cddf3c2c8b279cbb6d9dc2..f842ecd7877a70ea47e38b60c03b7de47c15a5bc 100644 (file)
@@ -114,7 +114,7 @@ func TestPageSortReverse(t *testing.T) {
        assert.Equal(t, 9, p2[0].fuzzyWordCount)
        assert.Equal(t, 0, p2[9].fuzzyWordCount)
        // cached
-       assert.True(t, fastEqualPages(p2, p1.Reverse()))
+       assert.True(t, pagesEqual(p2, p1.Reverse()))
 }
 
 func TestPageSortByParam(t *testing.T) {
index 665019bb4a5392fd73d86fda4fa6c1e64aac0f84..2881a45e6e3cb2eea235f7d57659beebaf5ecabd 100644 (file)
@@ -154,7 +154,7 @@ func newSearchIndexHandler(cfg related.Config) *relatedDocsHandler {
 // This assumes that a lock has been acquired.
 func (s *relatedDocsHandler) getIndex(p Pages) *related.InvertedIndex {
        for _, ci := range s.postingLists {
-               if fastEqualPages(p, ci.p) {
+               if pagesEqual(p, ci.p) {
                        return ci.postingList
                }
        }