Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sync internal commits 2024-09-06 #75

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

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.
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.

6 participants