From c3ed12c20ebe09c12efc431bfcd9666f1c588305 Mon Sep 17 00:00:00 2001 From: Travis Date: Mon, 6 Mar 2017 11:06:22 -0600 Subject: [PATCH] Refactor fragment.bitmap() so that it leverages `bitmapCache` and so that it's no longer reponsible for updating the count cache. This commit also helps SetBit/ClearBit performance by allowing them to work against data from `bitmapCache` instead of loading bitmaps from fragment.storage every time. --- bitmap.go | 2 -- fragment.go | 72 ++++++++++++++++++++++------------------------ roaring/roaring.go | 2 +- 3 files changed, 35 insertions(+), 41 deletions(-) diff --git a/bitmap.go b/bitmap.go index 651950e45..3a2e4c96a 100644 --- a/bitmap.go +++ b/bitmap.go @@ -16,8 +16,6 @@ type Bitmap struct { // Attributes associated with the bitmap. Attrs map[string]interface{} - - adjustedCount uint64 } // NewBitmap returns a new instance of Bitmap. diff --git a/fragment.go b/fragment.go index 8f8698192..51259585a 100644 --- a/fragment.go +++ b/fragment.go @@ -245,7 +245,7 @@ func (f *Fragment) openCache() error { // This will cause them to be added to the cache. for _, bitmapID := range pb.BitmapIDs { //n := f.storage.CountRange(bitmapID*SliceWidth, (bitmapID+1)*SliceWidth) - n := f.bitmap(bitmapID, false).Count() + n := f.bitmap(bitmapID, true, false).Count() f.cache.BulkAdd(bitmapID, n) } f.cache.Invalidate() @@ -312,33 +312,35 @@ func (f *Fragment) logger() *log.Logger { return log.New(f.LogOutput, "", log.Ls func (f *Fragment) Bitmap(bitmapID uint64) *Bitmap { f.mu.Lock() defer f.mu.Unlock() - return f.bitmap(bitmapID, false) + return f.bitmap(bitmapID, true, true) } -func (f *Fragment) bitmap(bitmapID uint64, updateCache bool) *Bitmap { - r, ok := f.bitmapCache.Fetch(bitmapID) - if ok && r != nil { - return r +func (f *Fragment) bitmap(bitmapID uint64, checkBitmapCache bool, updateBitmapCache bool) *Bitmap { + + if checkBitmapCache { + r, ok := f.bitmapCache.Fetch(bitmapID) + if ok && r != nil { + return r + } } + // Only use a subset of the containers. // NOTE: The start & end ranges must be divisible by data := f.storage.OffsetRange(f.slice*SliceWidth, bitmapID*SliceWidth, (bitmapID+1)*SliceWidth) // Reference bitmap subrange in storage. + // We Clone() data because otherwise bm will contains pointers to containers in storage. + // This causes unexpected results when we cache the bitmap and try to use it later. bm := &Bitmap{ segments: []BitmapSegment{{ - data: *data, + data: *data.Clone(), slice: f.slice, writable: false, }}, - adjustedCount: 0, } bm.InvalidateCount() - bm.adjustedCount = bm.Count() - if updateCache { - // Update cache. - f.cache.Add(bitmapID, bm.Count()) + if updateBitmapCache { f.bitmapCache.Add(bitmapID, bm) } @@ -353,9 +355,9 @@ func (f *Fragment) SetBit(bitmapID, profileID uint64) (changed bool, err error) return f.setBit(bitmapID, profileID) } -func (f *Fragment) setBit(bitmapID, profileID uint64) (changed bool, bool error) { - // Determine the position of the bit in the storage. +func (f *Fragment) setBit(bitmapID, profileID uint64) (changed bool, err error) { changed = false + // Determine the position of the bit in the storage. pos, err := f.pos(bitmapID, profileID) if err != nil { return false, err @@ -374,22 +376,18 @@ func (f *Fragment) setBit(bitmapID, profileID uint64) (changed bool, bool error) // Invalidate block checksum. delete(f.checksums, int(bitmapID/HashBlockSize)) - // If the number of operations exceeds the limit then snapshot. + // Increment number of operations until snapshot is required. if err := f.incrementOpN(); err != nil { return false, err } - // If adjustedCount is set, then apply that value to bitmap.n instead. - bm := f.bitmap(bitmapID, true) - if bm.adjustedCount > 0 { - bm.SetCount(profileID, bm.adjustedCount) - bm.adjustedCount = 0 - } else { - bm.IncrementCount(profileID) - f.cache.Add(bitmapID, bm.Count()) - } + // Get the bitmap from bitmapCache or fragment.storage. + bm := f.bitmap(bitmapID, true, true) bm.SetBit(profileID) + // Update the cache. + f.cache.Add(bitmapID, bm.Count()) + f.stats.Count("setN", 1) return changed, nil @@ -403,7 +401,8 @@ func (f *Fragment) ClearBit(bitmapID, profileID uint64) (bool, error) { return f.clearBit(bitmapID, profileID) } -func (f *Fragment) clearBit(bitmapID, profileID uint64) (bool, error) { +func (f *Fragment) clearBit(bitmapID, profileID uint64) (changed bool, err error) { + changed = false // Determine the position of the bit in the storage. pos, err := f.pos(bitmapID, profileID) if err != nil { @@ -411,8 +410,7 @@ func (f *Fragment) clearBit(bitmapID, profileID uint64) (bool, error) { } // Write to storage. - changed, err := f.storage.Remove(pos) - if err != nil { + if changed, err = f.storage.Remove(pos); err != nil { return false, err } @@ -429,17 +427,13 @@ func (f *Fragment) clearBit(bitmapID, profileID uint64) (bool, error) { return false, err } - // If adjustedCount is set, then apply that value to bitmap.n instead. - bm := f.bitmap(bitmapID, true) - if bm.adjustedCount > 0 { - bm.SetCount(profileID, bm.adjustedCount) - bm.adjustedCount = 0 - } else { - bm.DecrementCount(profileID) - f.cache.Add(bitmapID, bm.Count()) - } + // Get the bitmap from bitmapCache or fragment.storage. + bm := f.bitmap(bitmapID, true, true) bm.ClearBit(profileID) + // Update the cache. + f.cache.Add(bitmapID, bm.Count()) + f.stats.Count("clearN", 1) return changed, nil @@ -908,7 +902,10 @@ func (f *Fragment) Import(bitmapIDs, profileIDs []uint64) error { // Update cache counts for all bitmaps. for bitmapID := range set { - f.cache.BulkAdd(bitmapID, f.bitmap(bitmapID, false).Count()) + // Import should ALWAYS have bitmap() load a new bm from fragment.storage + // because the bitmap that's in bitmapCache hasn't been updated with + // this import's data. + f.cache.BulkAdd(bitmapID, f.bitmap(bitmapID, false, false).Count()) } f.cache.Invalidate() @@ -1256,7 +1253,6 @@ func (s *FragmentSyncer) SyncFragment() error { // Determine replica set. nodes := s.Cluster.FragmentNodes(s.Fragment.DB(), s.Fragment.Slice()) if len(nodes) == 1 { - //fmt.Println("no place to replicate", s.Fragment.DB(), s.Fragment.Frame(), s.Fragment.Slice()) return nil } diff --git a/roaring/roaring.go b/roaring/roaring.go index 58b4d1e19..e74739311 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -1084,7 +1084,7 @@ func (c *container) clone() *container { copy(other.bitmap, c.bitmap) } - return c + return other } // WriteTo writes c to w.