Add logic for Allocation structs for max usage

I achieved this by splitting Allocation addition logic
into aggregation and accumulation types, where:

Accumulation-type addition is "same resource, different response"
like getting multiple usage responses for the same container.

Aggregation-type addition is "same aggregation category," like
combining two containers under the same namespace.

Previously, both cases could be handled very easily, as we only
needed to add in both cases. With the introduction of max
CPU and RAM usage, we need to split the behavior because
the correct way to "add" max depends on accumulation vs.
aggregation. See code comments for further explanations and
implementation details.

pkg/kubecost/allocation.go

@@ -3,6 +3,7 @@ package kubecost
 import (
 	"bytes"
 	"fmt"
+	"math"
 	"sort"
 	"strings"
 	"sync"
@@ -79,10 +80,11 @@ type Allocation struct {
 // returning true for any given Allocation if a condition is met.
 type AllocationMatchFunc func(*Allocation) bool
 
-// Add returns the result of summing the two given Allocations, which sums the
-// summary fields (e.g. costs, resources) and recomputes efficiency. Neither of
-// the two original Allocations are mutated in the process.
-func (a *Allocation) Add(that *Allocation) (*Allocation, error) {
+// AddAccumulate returns the result of combining the two given Allocations,
+// under the assumption that they refer to the same resource. Costs (and
+// similar fields) are summed, usage maximums are MAXED, and efficiency is
+// recomputed. Neither of the original Allocations are mutated.
+func (a *Allocation) AddAccumulate(that *Allocation) (*Allocation, error) {
 	if a == nil {
 		return that.Clone(), nil
 	}
@@ -93,7 +95,33 @@ func (a *Allocation) Add(that *Allocation) (*Allocation, error) {
 
 	// Note: no need to clone "that", as add only mutates the receiver
 	agg := a.Clone()
-	agg.add(that)
+	agg.add(that, accumulation)
+
+	return agg, nil
+}
+
+// AddAggregate returns the result of combining the two given Allocations,
+// under the assumption that they refer to different resources. The intended
+// usage is e.g. "aggregate containers by namespace" where the resulting
+// allocation ultimately refers to a greater number of resources than either
+// of the original Allocations. Most fields, including usage maximums, are
+// summed. Neither of the original allocations are mutated.
+//
+// To explain why usage maximums are summed, consider using this method for
+// aggregating by pod. The maximum CPU usage of a pod can be roughly equated
+// to the sum of the maximum CPU usages of its containers.
+func (a *Allocation) AddAggregate(that *Allocation) (*Allocation, error) {
+	if a == nil {
+		return that.Clone(), nil
+	}
+
+	if that == nil {
+		return a.Clone(), nil
+	}
+
+	// Note: no need to clone "that", as add only mutates the receiver
+	agg := a.Clone()
+	agg.add(that, aggregation)
 
 	return agg, nil
 }
@@ -365,7 +393,21 @@ func (a *Allocation) String() string {
 	return fmt.Sprintf("%s%s=%.2f", a.Name, NewWindow(&a.Start, &a.End), a.TotalCost())
 }
 
-func (a *Allocation) add(that *Allocation) {
+// allocationCombinationType is an enum-like value for
+// flagging what mode an operation that combines allocations,
+// like add(), should operate in.
+//
+// Intentionally unexported - it should only be used by internal
+// methods. Exported methods should have a different variant,
+// like AddAccumulate and AddAggregate, for API clarity.
+type allocationCombinationType int
+
+const (
+	accumulation allocationCombinationType = iota
+	aggregation
+)
+
+func (a *Allocation) add(that *Allocation, addType allocationCombinationType) {
 	if a == nil {
 		log.Warningf("Allocation.AggregateBy: trying to add a nil receiver")
 		return
@@ -447,6 +489,19 @@ func (a *Allocation) add(that *Allocation) {
 	a.LoadBalancerCost += that.LoadBalancerCost
 	a.SharedCost += that.SharedCost
 	a.ExternalCost += that.ExternalCost
+
+	if addType == accumulation {
+		// Accumulation-type additions imply same resource, so the overall
+		// maximum is the correct resulting value.
+		a.CPUCoreUsageMax = math.Max(a.CPUCoreUsageMax, that.CPUCoreUsageMax)
+		a.RAMBytesUsageMax = math.Max(a.RAMBytesUsageMax, that.RAMBytesUsageMax)
+	} else if addType == aggregation {
+		// Aggregation-type additions imply different resources, same
+		// aggregation group, so the correct maximum is actually the sum
+		// of maxima.
+		a.CPUCoreUsageMax += that.CPUCoreUsageMax
+		a.RAMBytesUsageMax += that.RAMBytesUsageMax
+	}
 }
 
 // AllocationSet stores a set of Allocations, each with a unique name, that share
@@ -472,7 +527,7 @@ func NewAllocationSet(start, end time.Time, allocs ...*Allocation) *AllocationSe
 	}
 
 	for _, a := range allocs {
-		as.Insert(a)
+		as.InsertAccumulate(a)
 	}
 
 	return as
@@ -567,7 +622,7 @@ func (as *AllocationSet) AggregateBy(properties Properties, options *AllocationA
 		if alloc.IsExternal() {
 			delete(as.externalKeys, alloc.Name)
 			delete(as.allocations, alloc.Name)
-			externalSet.Insert(alloc)
+			externalSet.InsertAggregate(alloc)
 			continue
 		}
 
@@ -579,9 +634,9 @@ func (as *AllocationSet) AggregateBy(properties Properties, options *AllocationA
 			delete(as.allocations, alloc.Name)
 
 			if options.ShareIdle == ShareEven || options.ShareIdle == ShareWeighted {
-				idleSet.Insert(alloc)
+				idleSet.InsertAggregate(alloc)
 			} else {
-				aggSet.Insert(alloc)
+				aggSet.InsertAggregate(alloc)
 			}
 
 			continue
@@ -594,7 +649,7 @@ func (as *AllocationSet) AggregateBy(properties Properties, options *AllocationA
 			if sf(alloc) {
 				delete(as.idleKeys, alloc.Name)
 				delete(as.allocations, alloc.Name)
-				shareSet.Insert(alloc)
+				shareSet.InsertAggregate(alloc)
 				break
 			}
 		}
@@ -698,7 +753,7 @@ func (as *AllocationSet) AggregateBy(properties Properties, options *AllocationA
 
 			totalSharedCost := cost * hours
 
-			shareSet.Insert(&Allocation{
+			shareSet.InsertAggregate(&Allocation{
 				Name:       fmt.Sprintf("%s/%s", name, SharedSuffix),
 				Start:      as.Start(),
 				End:        as.End(),
@@ -800,7 +855,7 @@ func (as *AllocationSet) AggregateBy(properties Properties, options *AllocationA
 
 		// Inserting the allocation with the generated key for a name will
 		// perform the actual basic aggregation step.
-		aggSet.Insert(alloc)
+		aggSet.InsertAggregate(alloc)
 	}
 
 	// (6) If idle is shared and resources are shared, it's possible that some
@@ -927,7 +982,7 @@ func (as *AllocationSet) AggregateBy(properties Properties, options *AllocationA
 			key := alloc.generateKey(properties)
 
 			alloc.Name = key
-			aggSet.Insert(alloc)
+			aggSet.InsertAggregate(alloc)
 		}
 	}
 
@@ -936,7 +991,7 @@ func (as *AllocationSet) AggregateBy(properties Properties, options *AllocationA
 		for _, idleAlloc := range aggSet.IdleAllocations() {
 			aggSet.Delete(idleAlloc.Name)
 			idleAlloc.Name = IdleSuffix
-			aggSet.Insert(idleAlloc)
+			aggSet.InsertAggregate(idleAlloc)
 		}
 	}
 
@@ -1562,14 +1617,35 @@ func (as *AllocationSet) IdleAllocations() map[string]*Allocation {
 	return idles
 }
 
-// Insert aggregates the current entry in the AllocationSet by the given Allocation,
-// but only if the Allocation is valid, i.e. matches the AllocationSet's window. If
-// there is no existing entry, one is created. Nil error response indicates success.
-func (as *AllocationSet) Insert(that *Allocation) error {
-	return as.insert(that)
-}
-
-func (as *AllocationSet) insert(that *Allocation) error {
+// InsertAggregate aggregates the current entry in the AllocationSet by the given
+// Allocation, but only if the Allocation is valid, i.e. matches the AllocationSet's
+// window.
+// If there is no existing entry, one is created.
+// If there is an existing entry, the given Allocation is added to it using
+// aggregation logic, see Allocation.AddAggregate() for an explanation.
+// Nil error response indicates success.
+//
+// A good heuristic for whether you should use this method is:
+// Are you generating a new, less-specific key for a new Allocation from multiple
+// more specific Allocations?
+func (as *AllocationSet) InsertAggregate(that *Allocation) error {
+	return as.insert(that, aggregation)
+}
+
+// InsertAccumulate aggregates the current entry in the AllocationSet by the given
+// Allocation, but only if the Allocation is valid, i.e. matches the AllocationSet's
+// window.
+// If there is no existing entry, one is created.
+// If there is an existing entry, the given Allocation is added to it using
+// accumulation logic, see Allocation.AddAccumulate() for an explanation.
+// Nil error response indicates success.
+//
+// If you don't know which insert to use, this is a safe default.
+func (as *AllocationSet) InsertAccumulate(that *Allocation) error {
+	return as.insert(that, accumulation)
+}
+
+func (as *AllocationSet) insert(that *Allocation, combType allocationCombinationType) error {
 	if as == nil {
 		return fmt.Errorf("cannot insert into nil AllocationSet")
 	}
@@ -1594,7 +1670,7 @@ func (as *AllocationSet) insert(that *Allocation) error {
 	if _, ok := as.allocations[that.Name]; !ok {
 		as.allocations[that.Name] = that
 	} else {
-		as.allocations[that.Name].add(that)
+		as.allocations[that.Name].add(that, combType)
 	}
 
 	// If the given Allocation is an external one, record that
@@ -1755,14 +1831,14 @@ func (as *AllocationSet) accumulate(that *AllocationSet) (*AllocationSet, error)
 	defer that.RUnlock()
 
 	for _, alloc := range as.allocations {
-		err := acc.insert(alloc)
+		err := acc.insert(alloc, accumulation)
 		if err != nil {
 			return nil, err
 		}
 	}
 
 	for _, alloc := range that.allocations {
-		err := acc.insert(alloc)
+		err := acc.insert(alloc, accumulation)
 		if err != nil {
 			return nil, err
 		}
@@ -1906,7 +1982,7 @@ func (asr *AllocationSetRange) InsertRange(that *AllocationSetRange) error {
 
 		// Insert each Allocation from the given set
 		thatAS.Each(func(k string, alloc *Allocation) {
-			err = as.Insert(alloc)
+			err = as.InsertAccumulate(alloc)
 			if err != nil {
 				err = fmt.Errorf("error inserting allocation: %s", err)
 				return

pkg/kubecost/allocation_test.go

@@ -69,12 +69,59 @@ func NewUnitAllocation(name string, start time.Time, resolution time.Duration, p
 	return alloc
 }
 
-func TestAllocation_Add(t *testing.T) {
+// Because it is the standard case, the test for AddAccumulate is expected
+// to test all Add behavior except for Aggregate-specific behavior. This test
+// can therefore be somewhat minimal.
+func TestAllocation_AddAggregate(t *testing.T) {
+	s1 := time.Date(2021, time.January, 1, 0, 0, 0, 0, time.UTC)
+	e1 := time.Date(2021, time.January, 1, 12, 0, 0, 0, time.UTC)
+	gib := 1024.0 * 1024.0 * 1024.0
+	a1 := &Allocation{
+		Start:            s1,
+		End:              e1,
+		CPUCoreUsageMax:  3.1,
+		RAMBytesUsageMax: 5.0 * gib,
+	}
+	a1b := a1.Clone()
+
+	s2 := time.Date(2021, time.January, 1, 6, 0, 0, 0, time.UTC)
+	e2 := time.Date(2021, time.January, 1, 24, 0, 0, 0, time.UTC)
+	a2 := &Allocation{
+		Start:            s2,
+		End:              e2,
+		CPUCoreUsageMax:  1.2,
+		RAMBytesUsageMax: 10.0 * gib,
+	}
+	a2b := a2.Clone()
+
+	act, err := a1.AddAggregate(a2)
+	if err != nil {
+		t.Fatalf("Allocation.AddAggregate: unexpected error: %s", err)
+	}
+
+	// Neither Allocation should be mutated
+	if !a1.Equal(a1b) {
+		t.Fatalf("Allocation.AddAccumulate: a1 illegally mutated")
+	}
+	if !a2.Equal(a2b) {
+		t.Fatalf("Allocation.AddAccumulate: a1 illegally mutated")
+	}
+
+	// Usage maximums should be added in the aggregate case
+	if !util.IsApproximately(3.1+1.2, act.CPUCoreUsageMax) {
+		t.Errorf("Allocation.AddAggregate: CPUCoreUsageMax: expected %f; actual %f", 3.1+1.2, act.CPUCoreUsageMax)
+	}
+	if !util.IsApproximately(10.0*gib+5.0*gib, act.RAMBytesUsageMax) {
+		t.Errorf("Allocation.AddAggregate: RAMBytesUsageMax: expected %f; actual %f", 10.0*gib+5.0*gib, act.RAMBytesUsageMax)
+	}
+}
+
+func TestAllocation_AddAccumulate(t *testing.T) {
 	var nilAlloc *Allocation
 	zeroAlloc := &Allocation{}
 
 	// nil + nil == nil
-	nilNilSum, err := nilAlloc.Add(nilAlloc)
+	nilNilSum, err := nilAlloc.AddAccumulate(nilAlloc)
 	if err != nil {
 		t.Fatalf("Allocation.Add unexpected error: %s", err)
 	}
@@ -83,7 +130,7 @@ func TestAllocation_Add(t *testing.T) {
 	}
 
 	// nil + zero == zero
-	nilZeroSum, err := nilAlloc.Add(zeroAlloc)
+	nilZeroSum, err := nilAlloc.AddAccumulate(zeroAlloc)
 	if err != nil {
 		t.Fatalf("Allocation.Add unexpected error: %s", err)
 	}
@@ -106,6 +153,7 @@ func TestAllocation_Add(t *testing.T) {
 		CPUCoreHours:           2.0 * hrs1,
 		CPUCoreRequestAverage:  2.0,
 		CPUCoreUsageAverage:    1.0,
+		CPUCoreUsageMax:        3.1,
 		CPUCost:                2.0 * hrs1 * cpuPrice,
 		GPUHours:               1.0 * hrs1,
 		GPUCost:                1.0 * hrs1 * gpuPrice,
@@ -114,6 +162,7 @@ func TestAllocation_Add(t *testing.T) {
 		RAMByteHours:           8.0 * gib * hrs1,
 		RAMBytesRequestAverage: 8.0 * gib,
 		RAMBytesUsageAverage:   4.0 * gib,
+		RAMBytesUsageMax:       5.0 * gib,
 		RAMCost:                8.0 * hrs1 * ramPrice,
 		SharedCost:             2.00,
 		ExternalCost:           1.00,
@@ -129,6 +178,7 @@ func TestAllocation_Add(t *testing.T) {
 		CPUCoreHours:           1.0 * hrs2,
 		CPUCoreRequestAverage:  1.0,
 		CPUCoreUsageAverage:    1.0,
+		CPUCoreUsageMax:        1.2,
 		CPUCost:                1.0 * hrs2 * cpuPrice,
 		GPUHours:               0.0,
 		GPUCost:                0.0,
@@ -137,6 +187,7 @@ func TestAllocation_Add(t *testing.T) {
 		RAMByteHours:           8.0 * gib * hrs2,
 		RAMBytesRequestAverage: 0.0,
 		RAMBytesUsageAverage:   8.0 * gib,
+		RAMBytesUsageMax:       10.0 * gib,
 		RAMCost:                8.0 * hrs2 * ramPrice,
 		NetworkCost:            0.01,
 		LoadBalancerCost:       0.05,
@@ -145,65 +196,65 @@ func TestAllocation_Add(t *testing.T) {
 	}
 	a2b := a2.Clone()
 
-	act, err := a1.Add(a2)
+	act, err := a1.AddAccumulate(a2)
 	if err != nil {
-		t.Fatalf("Allocation.Add: unexpected error: %s", err)
+		t.Fatalf("Allocation.AddAccumulate: unexpected error: %s", err)
 	}
 
 	// Neither Allocation should be mutated
 	if !a1.Equal(a1b) {
-		t.Fatalf("Allocation.Add: a1 illegally mutated")
+		t.Fatalf("Allocation.AddAccumulate: a1 illegally mutated")
 	}
 	if !a2.Equal(a2b) {
-		t.Fatalf("Allocation.Add: a1 illegally mutated")
+		t.Fatalf("Allocation.AddAccumulate: a1 illegally mutated")
 	}
 
 	// Costs should be cumulative
 	if !util.IsApproximately(a1.TotalCost()+a2.TotalCost(), act.TotalCost()) {
-		t.Fatalf("Allocation.Add: expected %f; actual %f", a1.TotalCost()+a2.TotalCost(), act.TotalCost())
+		t.Fatalf("Allocation.AddAccumulate: expected %f; actual %f", a1.TotalCost()+a2.TotalCost(), act.TotalCost())
 	}
 	if !util.IsApproximately(a1.CPUCost+a2.CPUCost, act.CPUCost) {
-		t.Fatalf("Allocation.Add: expected %f; actual %f", a1.CPUCost+a2.CPUCost, act.CPUCost)
+		t.Fatalf("Allocation.AddAccumulate: expected %f; actual %f", a1.CPUCost+a2.CPUCost, act.CPUCost)
 	}
 	if !util.IsApproximately(a1.GPUCost+a2.GPUCost, act.GPUCost) {
-		t.Fatalf("Allocation.Add: expected %f; actual %f", a1.GPUCost+a2.GPUCost, act.GPUCost)
+		t.Fatalf("Allocation.AddAccumulate: expected %f; actual %f", a1.GPUCost+a2.GPUCost, act.GPUCost)
 	}
 	if !util.IsApproximately(a1.RAMCost+a2.RAMCost, act.RAMCost) {
-		t.Fatalf("Allocation.Add: expected %f; actual %f", a1.RAMCost+a2.RAMCost, act.RAMCost)
+		t.Fatalf("Allocation.AddAccumulate: expected %f; actual %f", a1.RAMCost+a2.RAMCost, act.RAMCost)
 	}
 	if !util.IsApproximately(a1.PVCost+a2.PVCost, act.PVCost) {
-		t.Fatalf("Allocation.Add: expected %f; actual %f", a1.PVCost+a2.PVCost, act.PVCost)
+		t.Fatalf("Allocation.AddAccumulate: expected %f; actual %f", a1.PVCost+a2.PVCost, act.PVCost)
 	}
 	if !util.IsApproximately(a1.NetworkCost+a2.NetworkCost, act.NetworkCost) {
-		t.Fatalf("Allocation.Add: expected %f; actual %f", a1.NetworkCost+a2.NetworkCost, act.NetworkCost)
+		t.Fatalf("Allocation.AddAccumulate: expected %f; actual %f", a1.NetworkCost+a2.NetworkCost, act.NetworkCost)
 	}
 	if !util.IsApproximately(a1.LoadBalancerCost+a2.LoadBalancerCost, act.LoadBalancerCost) {
-		t.Fatalf("Allocation.Add: expected %f; actual %f", a1.LoadBalancerCost+a2.LoadBalancerCost, act.LoadBalancerCost)
+		t.Fatalf("Allocation.AddAccumulate: expected %f; actual %f", a1.LoadBalancerCost+a2.LoadBalancerCost, act.LoadBalancerCost)
 	}
 	if !util.IsApproximately(a1.SharedCost+a2.SharedCost, act.SharedCost) {
-		t.Fatalf("Allocation.Add: expected %f; actual %f", a1.SharedCost+a2.SharedCost, act.SharedCost)
+		t.Fatalf("Allocation.AddAccumulate: expected %f; actual %f", a1.SharedCost+a2.SharedCost, act.SharedCost)
 	}
 	if !util.IsApproximately(a1.ExternalCost+a2.ExternalCost, act.ExternalCost) {
-		t.Fatalf("Allocation.Add: expected %f; actual %f", a1.ExternalCost+a2.ExternalCost, act.ExternalCost)
+		t.Fatalf("Allocation.AddAccumulate: expected %f; actual %f", a1.ExternalCost+a2.ExternalCost, act.ExternalCost)
 	}
 
 	// ResourceHours should be cumulative
 	if !util.IsApproximately(a1.CPUCoreHours+a2.CPUCoreHours, act.CPUCoreHours) {
-		t.Fatalf("Allocation.Add: expected %f; actual %f", a1.CPUCoreHours+a2.CPUCoreHours, act.CPUCoreHours)
+		t.Fatalf("Allocation.AddAccumulate: expected %f; actual %f", a1.CPUCoreHours+a2.CPUCoreHours, act.CPUCoreHours)
 	}
 	if !util.IsApproximately(a1.RAMByteHours+a2.RAMByteHours, act.RAMByteHours) {
-		t.Fatalf("Allocation.Add: expected %f; actual %f", a1.RAMByteHours+a2.RAMByteHours, act.RAMByteHours)
+		t.Fatalf("Allocation.AddAccumulate: expected %f; actual %f", a1.RAMByteHours+a2.RAMByteHours, act.RAMByteHours)
 	}
 	if !util.IsApproximately(a1.PVByteHours+a2.PVByteHours, act.PVByteHours) {
-		t.Fatalf("Allocation.Add: expected %f; actual %f", a1.PVByteHours+a2.PVByteHours, act.PVByteHours)
+		t.Fatalf("Allocation.AddAccumulate: expected %f; actual %f", a1.PVByteHours+a2.PVByteHours, act.PVByteHours)
 	}
 
 	// Minutes should be the duration between min(starts) and max(ends)
 	if !act.Start.Equal(a1.Start) || !act.End.Equal(a2.End) {
-		t.Fatalf("Allocation.Add: expected %s; actual %s", NewWindow(&a1.Start, &a2.End), NewWindow(&act.Start, &act.End))
+		t.Fatalf("Allocation.AddAccumulate: expected %s; actual %s", NewWindow(&a1.Start, &a2.End), NewWindow(&act.Start, &act.End))
 	}
 	if act.Minutes() != 1440.0 {
-		t.Fatalf("Allocation.Add: expected %f; actual %f", 1440.0, act.Minutes())
+		t.Fatalf("Allocation.AddAccumulate: expected %f; actual %f", 1440.0, act.Minutes())
 	}
 
 	// Requests and Usage should be averaged correctly
@@ -212,16 +263,24 @@ func TestAllocation_Add(t *testing.T) {
 	// RAM requests = (8.0*12.0 + 0.0*18.0)/(24.0) = 4.00
 	// RAM usage = (4.0*12.0 + 8.0*18.0)/(24.0) = 8.00
 	if !util.IsApproximately(1.75, act.CPUCoreRequestAverage) {
-		t.Fatalf("Allocation.Add: expected %f; actual %f", 1.75, act.CPUCoreRequestAverage)
+		t.Fatalf("Allocation.AddAccumulate: expected %f; actual %f", 1.75, act.CPUCoreRequestAverage)
 	}
 	if !util.IsApproximately(1.25, act.CPUCoreUsageAverage) {
-		t.Fatalf("Allocation.Add: expected %f; actual %f", 1.25, act.CPUCoreUsageAverage)
+		t.Fatalf("Allocation.AddAccumulate: expected %f; actual %f", 1.25, act.CPUCoreUsageAverage)
 	}
 	if !util.IsApproximately(4.00*gib, act.RAMBytesRequestAverage) {
-		t.Fatalf("Allocation.Add: expected %f; actual %f", 4.00*gib, act.RAMBytesRequestAverage)
+		t.Fatalf("Allocation.AddAccumulate: expected %f; actual %f", 4.00*gib, act.RAMBytesRequestAverage)
 	}
 	if !util.IsApproximately(8.00*gib, act.RAMBytesUsageAverage) {
-		t.Fatalf("Allocation.Add: expected %f; actual %f", 8.00*gib, act.RAMBytesUsageAverage)
+		t.Fatalf("Allocation.AddAccumulate: expected %f; actual %f", 8.00*gib, act.RAMBytesUsageAverage)
+	}
+
+	// Usage maximums should be maxed in the accumulate case
+	if !util.IsApproximately(3.1, act.CPUCoreUsageMax) {
+		t.Errorf("Allocation.AddAccumulate: CPUCoreUsageMax: expected %f; actual %f", 3.1, act.CPUCoreUsageMax)
+	}
+	if !util.IsApproximately(10.0*gib, act.RAMBytesUsageMax) {
+		t.Errorf("Allocation.AddAccumulate: RAMBytesUsageMax: expected %f; actual %f", 10.0*gib, act.RAMBytesUsageMax)
 	}
 
 	// Efficiency should be computed accurately from new request/usage