--cap-mbps refactor #3122

adreed-msft · 2025-07-16T04:21:25Z

This one is gonna be a bit lengthy, and I'm still sorta in the depths of it.

At this point, most of the meat and potatoes are here, but this still isn't quite ready to go. It won't build yet. I'm reworking the page blob auto pacer around the new model. At that point, things need to be plugged in.

It's important to note, before review, what's majorly different:

The pacer is implemented as a pipeline policy now.
The pacer now operates at a fine-grained level, allowing requests based upon the ability to meet minimum throughput requirements, with a "red line" before it starts sharing remaining bandwidth evenly. THIS WILL MOST LIKELY REQUIRE TUNING.
This doesn't entirely apply for S2S, since we can't possibly pace that. Thus, it just allocates the blocks gradually similarly to the old way.
To apply the pipeline policy to page blobs, we need an injector to smuggle the policy in for us. That is one of the new policies.

I need to write tests for no bandwidth limit as well, because I don't trust that quite yet.

gapra-msft · 2025-07-16T17:04:54Z

ste/pacer-autoPacer.go

-	a.logger.Log(common.LogInfo, fmt.Sprintf("%s: Target Mbps %d", a.logPrefix, (a.targetBytesPerSecond()*8)/(1000*1000)))
-}
+//
+//var (


Can we just delete this file?

gapra-msft · 2025-07-16T17:10:43Z

common/yield_send.go

+package common
+
+// NonBlockingSafeSend implements a channel send that is guaranteed to not panic, and may be either instantaneous or waiting.
+// Instantaneous returns false if a panic occurred, or if


this comment is incomplete.

gapra-msft · 2025-07-16T17:33:50Z

ste/pacer-RequestPolicyPacer-pacer.go

+		newAlloc uint64
+	} // when a request seeks, it should fire itself down this channel to manage its allocation
+
+	shutdownCh chan bool


I don't see this being initialized in the constructor.

dphulkar-msft · 2025-07-17T06:42:19Z

ste/pacedReadSeeker.go

@@ -1,85 +1,85 @@
-// Copyright © 2017 Microsoft <[email protected]>
+// // Copyright © 2017 Microsoft <[email protected]>


I noticed that the entire file is commented out. If it's no longer needed, should we consider removing it?

dphulkar-msft · 2025-07-17T06:44:05Z

ste/pacer-NullRequestPolicyPacer.go

@@ -0,0 +1,22 @@
+package ste


Since this is a new file, we should add the Microsoft licensing header.

dphulkar-msft · 2025-07-17T06:44:21Z

ste/pacer-RequestPolicyPacer-interface.go

@@ -0,0 +1,31 @@
+package ste


same as above comment

dphulkar-msft · 2025-07-17T06:45:12Z

ste/pacer-RequestPolicyPacer-pacer.go

@@ -0,0 +1,373 @@
+package ste


dphulkar-msft · 2025-07-17T06:49:54Z

ste/pacer-RequestPolicyPacer-pacer.go

+		isLive:                  &atomic.Pointer[bool]{},
+
+		liveBodies:         make(map[uuid.UUID]*policyPacerBody),
+		requestInitChannel: make(chan *policyPacerBody, 300),


we can declare this as constant defaultBufferSize=300

dphulkar-msft · 2025-07-17T07:19:32Z

ste/pacer-RequestPolicyPacer-pacer.go

+			common.AtomicSubtract(r.allocatedBytesPerSecond, PacerBodyMinimumBytesPerSecond)
+			req.bodyStatus.Store(to.Ptr(BodyStatusDeallocated))
+		default:
+			escape = true // break doesn't work in here


we can do something like this.

for { select { case req := <-r.pacerExitChannel: delete(r.liveBodies, req.id) common.AtomicSubtract(r.allocatedBytesPerSecond, PacerBodyMinimumBytesPerSecond) req.bodyStatus.Store(to.Ptr(BodyStatusDeallocated)) default: return } }

dphulkar-msft · 2025-07-17T07:24:28Z

ste/pacer-RequestPolicyPacer-pacer.go

+			// Inform the body that it is allocated
+			newResponse.bodyStatus.Store(to.Ptr(BodyStatusAllocated))
+		default:
+			escape = true


same as above

dphulkar-msft · 2025-07-17T07:26:19Z

ste/pacer-RequestPolicyPacer-pacer.go

+				// Inform the body that it is allocated
+				newRequest.bodyStatus.Store(to.Ptr(BodyStatusCompleted))
+			default: // there's nothing new, so let's use what we have.
+				escape = true


same as above

dphulkar-msft · 2025-07-17T07:26:28Z

ste/pacer-RequestPolicyPacer-pacer.go

+				// Inform the body that it is allocated
+				newRequest.bodyStatus.Store(to.Ptr(BodyStatusAllocated))
+			default: // there's nothing new, so let's use what we have.
+				escape = true


dphulkar-msft · 2025-07-17T09:49:36Z

ste/pacer-nullPolicyPacer.go

+
+	if resp != nil {
+		n.parent.ProcessBytes(uint64(max(resp.ContentLength, 0)))
+	} else if err == nil {


Question: Can both resp and err be nil? Normally, if err is nil, resp is expected to be non-nil.
However, it's good defensive coding to handle the rare case where a misbehaving policy or transport layer might return both as nil.
In such a case, we safely return the previously allocated bytes.

dphulkar-msft · 2025-07-17T09:51:16Z

ste/pacer-policy-autoPacer.go

+
+	maxPacerGbps           = 100
+	maxPacerBytesPerSecond = maxPacerGbps * 1000 * 1000 * 1000 / 8
+)


I've noticed that there are many constants defined across different files in the ste folder. Would it make sense to create a dedicated constants.go file under ste to centralize and manage all constants for better maintainability?

dphulkar-msft · 2025-07-17T09:52:40Z

ste/pacer-policy-autoPacer.go

+	pacer := NewRequestPolicyPacer(targetBytesPerSecond)
+
+	return &RequestPolicyAutoPacer{
+		RequestPolicyPacer: pacer,


nit pick: RequestPolicyPacer: NewRequestPolicyPacer(targetBytesPerSecond),

dphulkar-msft · 2025-07-17T09:55:17Z

ste/pacer-policy-autoPacer.go

+	corePolicy := r.RequestPolicyPacer.GetPolicy()
+
+	return &autoPacerPolicy{
+		p:      corePolicy,


Should we consider using a more meaningful variable name instead of p? Something like innerPolicy, wrappedPolicy, or corePolicy

dphulkar-msft · 2025-07-17T09:57:00Z

ste/pacer-requestPolicyPacer-body.go

@@ -0,0 +1,302 @@
+package ste


msft licensing header required

dphulkar-msft · 2025-07-17T09:57:24Z

ste/pacer-requestPolicyPacer-body.go

+	BodyStatusNew         BodyStatus = 0
+	BodyStatusAllocated   BodyStatus = 1
+	BodyStatusCompleted   BodyStatus = 2
+	BodyStatusDeallocated BodyStatus = -1


This can be moved to a separate constants file

dphulkar-msft · 2025-07-17T10:09:01Z

ste/policy-conditionWrapper.go

+	if pol, ok := newPolicy.(policy.Policy); ok {
+		return pol.Do(req)
+	} else if pol != nil {
+		return nil, fmt.Errorf("supplied policy for key %v was not a policy.Policy but a %v", p.policyKey, newPolicy)


Since we are checking the type of the policy, should we use %T here to clearly indicate the actual type received?

adreed-msft added 2 commits July 11, 2025 18:13

Save state

5596d60

Still fairly WIP, but most of the meat and potatoes are present.

8869c71

gapra-msft reviewed Jul 16, 2025

View reviewed changes

dphulkar-msft reviewed Jul 17, 2025

View reviewed changes

ste/pacer-RequestPolicyPacer-interface.go

@@ -0,0 +1,31 @@

package ste

Copy link

Member

dphulkar-msft Jul 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above comment

dphulkar-msft reviewed Jul 17, 2025

View reviewed changes

ste/pacer-RequestPolicyPacer-pacer.go

@@ -0,0 +1,373 @@

package ste

Copy link

Member

dphulkar-msft Jul 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

dphulkar-msft reviewed Jul 17, 2025

View reviewed changes

ste/pacer-requestPolicyPacer-body.go

@@ -0,0 +1,302 @@

package ste

Copy link

Member

dphulkar-msft Jul 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

msft licensing header required

dphulkar-msft reviewed Jul 17, 2025

View reviewed changes

		@@ -1,85 +1,85 @@
		// Copyright © 2017 Microsoft <[email protected]>
		// // Copyright © 2017 Microsoft <[email protected]>

--cap-mbps refactor #3122

Are you sure you want to change the base?

--cap-mbps refactor #3122

Uh oh!

Conversation

adreed-msft commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dphulkar-msft Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dphulkar-msft Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dphulkar-msft Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adreed-msft commented Jul 16, 2025 •

edited

Loading

dphulkar-msft Jul 17, 2025 •

edited

Loading

dphulkar-msft Jul 17, 2025 •

edited

Loading

dphulkar-msft Jul 17, 2025 •

edited

Loading