Performance improvement based on pprofile

[gab-arrobo]

No description provided.

[Copilot]

🟡 Not ready to approve

The new UdrUri caching introduces additional unsynchronized reads/writes of a shared field, increasing the risk of Go data races under concurrent request handling.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR aims to improve runtime performance by reducing repeated UDR discovery calls via caching and by lowering log verbosity on hot request paths.

Changes:

• Cache UdrUri in UE context to avoid repeated SendNFInstancesUDR calls for SUPI/PEI-based lookups.
• Downgrade multiple request entry logs from Info* to Debug* in UECM and SDM handlers/procedures.

File summaries

File	Description
producer/ue_context_management.go	Adds UdrUri reuse in getUdrURI and reduces handler/procedure log level to debug.
producer/subscriber_data_management.go	Reduces SDM handler/procedure log level to debug to cut logging overhead.

Review details

Suppressed comments (1)

producer/ue_context_management.go:72

• Same data-race risk here: ue.UdrUri is conditionally initialized and then read without synchronization inside UdmUePool.Range. Under concurrent access, reading/writing this string field can race. Recommend guarding UdrUri access with a lock or sync.Once-style initialization in UdmUeContext.

				if ue.UdrUri == "" {
					ue.UdrUri = consumer.SendNFInstancesUDR(ue.Supi, consumer.NFDiscoveryToUDRParamSupi)
				}
				udrURI = ue.UdrUri

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

It introduces a request-path bug/inconsistency (invalid pduSessionId returns 204 with no problem details) and includes a couple of correctness/quality issues that should be addressed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (3)

producer/ue_context_management.go:88

• Debugf is used without any formatting directives; this should be Debugln/Debug to avoid confusion and unnecessary formatting overhead.

func HandleGetAmf3gppAccessRequest(request *httpwrapper.Request) *httpwrapper.Response {
	logger.UecmLog.Debugf("Handle HandleGetAmf3gppAccessRequest")
	ueID := request.Params["ueId"]

producer/ue_context_management.go:487

• When pduSessionId parsing fails, the handler currently logs an error but returns 204 No Content with no problem details or failure stats, which can hide client-side request errors and make troubleshooting difficult. Return 400 Bad Request with a malformed-syntax ProblemDetails (and count as FAILURE) instead.

	pduSessionID, err := strconv.ParseInt(pduSessionIDStr, 10, 32)
	if err != nil {
		logger.UecmLog.Errorln("pduSessionID error:", err)
		return httpwrapper.NewResponse(http.StatusNoContent, nil, nil)
	}

context/context.go:92

• GetOrSetUdrUri adds new synchronization/caching behavior but there are no accompanying tests to ensure fetch is only invoked once under concurrent access and that subsequent calls return the cached value. Adding a small unit test (including a concurrency case) would help prevent regressions.

// GetOrSetUdrUri returns the cached UdrUri if already set; otherwise calls fetch, stores and returns the result.
// Uses double-checked locking to avoid unnecessary network calls under concurrent requests.
func (ue *UdmUeContext) GetOrSetUdrUri(fetch func() string) string {

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

The SMF registration procedure’s contextExisted logic is incorrect due to mutating context before checking existence (and before validating pduSessionId), which can force the wrong response behavior and persist invalid context state.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

context/context.go:92

• GetOrSetUdrUri introduces new concurrency/caching behavior but there is no unit test covering the expected semantics (e.g., fetch is only invoked once under concurrent callers, and subsequent calls return the cached value). Adding tests would help prevent regressions in this performance-critical path.

func (ue *UdmUeContext) GetOrSetUdrUri(fetch func() string) string {

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

The new UDR-URI caching behavior may repeatedly re-trigger NF discovery when discovery returns an empty URI, which can amplify load during NRF/UDR failure modes and should be explicitly decided/handled.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (2)

producer/ue_context_management.go:552

• This comment is inaccurate: CreateSmfRegContext only sets PduSessionID if it is currently empty (context/context.go:363-366), so it does not “always” set it. Updating the comment will avoid misleading future changes to this flow.

	// Check existence before CreateSmfRegContext; that call always sets PduSessionID.

context/context.go:97

• GetOrSetUdrUri only treats a non-empty UdrUri as cached. If the fetch returns an empty string (e.g., SendNFInstancesUDR returns "" when discovery fails or finds zero instances; see consumer/nf_discovery.go:171-177), every subsequent call will re-run NF discovery, which can amplify load/logging during NRF/UDR outages and undermines the intended caching under failure modes. Consider explicitly caching the “attempted” state (even when the result is empty) or adding a backoff/TTL policy so repeated failures don’t trigger repeated network calls.

	ue.udrUriLock.RLock()
	if ue.UdrUri != "" {
		uri := ue.UdrUri
		ue.udrUriLock.RUnlock()
		return uri

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[andybavier]

+1

[Copilot]

🟡 Not ready to approve

The new caching/retry behavior and concurrency logic lacks targeted tests and includes at least one misleading comment that should be corrected before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (3)

producer/ue_context_management.go:552

• The comment is misleading: CreateSmfRegContext only sets PduSessionID when it is currently empty, so it does not "always" set it. This affects how readers interpret the existence check.

	// Check existence before CreateSmfRegContext; that call always sets PduSessionID.

context/context.go:110

• GetOrSetUdrUri intentionally does not cache empty URIs; as written this will retry NRF discovery on every request when fetch() returns "", which can create sustained load during NRF/UDR outages. Consider adding a short-lived negative cache/backoff (e.g., cache empty with a timestamp/TTL) to avoid repeated discovery storms while still allowing recovery.

		if uri := fetch(); uri != "" {
			ue.UdrUri = uri
			return uri
		}
		return ""

context/context.go:94

• GetOrSetUdrUri introduces new concurrency-sensitive caching behavior (double-checked locking, single fetch under contention, and non-caching of empty results) but there are no unit tests covering these cases. Adding tests would help prevent regressions and validate the intended semantics.

// GetOrSetUdrUri returns the cached UdrUri if already set; otherwise calls fetch and caches the result.
// Uses double-checked locking to avoid unnecessary NRF discovery calls under concurrent requests.
// Empty URIs (NRF/UDR unreachable) are intentionally not cached so discovery is retried on the
// next request, allowing recovery when the NRF or UDR becomes available again.
func (ue *UdmUeContext) GetOrSetUdrUri(fetch func() string) string {

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

The new permanent UDR-URI caching removes re-discovery/invalidation behavior and risks pinning a UE to a stale/unhealthy UDR endpoint without a clear eviction strategy.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

producer/ue_context_management.go:54

• getUdrURI now permanently caches the first non-empty UDR URI per UE via GetOrSetUdrUri. This avoids repeated NRF discovery but also removes any chance to re-discover if the NRF result changes (e.g., UDR failover/relocation) or if the cached UDR URI becomes unhealthy; subsequent requests will keep using the stale URI because nothing ever invalidates UdrUri once set.

			return ue.GetOrSetUdrUri(func() string {
				return consumer.SendNFInstancesUDR(id, consumer.NFDiscoveryToUDRParamSupi)
			})

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟢 Ready to approve

The changes are localized, include targeted unit tests for the new caching behavior, and the remaining feedback is limited to a minor comment clarification.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

producer/ue_context_management.go:551

• The comment is inaccurate: CreateSmfRegContext only sets PduSessionID when it is currently empty (see context/context.go:371-379). Clarify this to avoid misleading future changes.

	// Check existence before CreateSmfRegContext; that call always sets PduSessionID.

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[andybavier]

+1