Skip to content

Conversation

@coco-speed
Copy link
Collaborator

This pull request was created automatically by CodSpeed to track performance changes of the pull request cloudflare/pingora#375.

The original branch is upstream/ewang/2024-09-06

andrewhavck and others added 7 commits August 27, 2024 09:41
If we have a cache miss, any meta in this object is invalid. Unset
it so that we don't use it later.
Includes-commit: 9a934bc
Replicated-from: cloudflare#360
This has the effect of reducing a warning log since we are not
leaving the lock dangling for the next reader.
This changes the memory ordering for the lock status load to `SeqCst` from
`Relaxed` to eliminate a potential source of panics.

Panics had the frames:
```
pingora_proxy::proxy_cache::<T>::handle_lock_status (proxy_cache.rs:748)
pingora_proxy::proxy_cache::<T>::proxy_cache::{{closure}} (proxy_cache.rs:211)
pingora_proxy::HttpProxy<T>::process_request::{{closure}} (lib.rs:509)
pingora_proxy::HttpProxy<T>::process_new_http::{{closure}} (lib.rs:727)
```

which showed we were checking on the status of the lock, after waiting on it,
and still seeing its status as waiting. The status is returned by value, so this
is not a time-of-check to time-of-use problem, this is an inconsistency in how
the lock status is managed. The change in memory order is mostly for the sake of
this programmer's attempts to understand what is happening.

This also completes a couple of TODOs to limit the wait period as well as tag
the span with the lock status.
@codspeed-hq
Copy link

codspeed-hq bot commented Sep 6, 2024

CodSpeed Performance Report

Merging #75 will not alter performance

Comparing upstream/ewang/2024-09-06 (1e0e0bc) with main (fa851d6)

Summary

✅ 2 untouched benchmarks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants