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

ws: Prep work for trailers support #4130

Merged
merged 2 commits into from
Jul 8, 2024
Merged

Conversation

dridi
Copy link
Member

@dridi dridi commented Jul 3, 2024

Originally from #4125, submitted independently as per bugwash to reduce the size of #4125.

dridi and others added 2 commits July 3, 2024 14:38
This puts the caller in charge of triggering the workspace rollback
before moving pipelined bytes around. This will open the door to
pipelining for req and beresp trailers.

The workspace emulator version of WS_ReqPipeline() had a systematic
rollback instead of a conditional one. The reason why this didn't pose
a problem was that fetch tasks never need pipelining and always take
the shortcut where the rollback was properly optional.

This is a good occasion to simplify the emulated variant and document
why it is so different from the original.

Co-authored-by: Walid Boudebouda <[email protected]>
The reset pointer is effectively the beginning of the reservation, so in
order to better generalize HEADERS frames processing between headers and
trailers, we reference the workspace directly.
@nigoroll
Copy link
Member

nigoroll commented Jul 8, 2024

The second (hpack) commit seems to make sense to me, but I do not understand the first: What are your plans with the pipeline logic? #4125 does not seem to make use of the conditional rollback and I do not understand what the meaning of a "pipeline at the end of the workspace" would be?

@dridi
Copy link
Member Author

dridi commented Jul 8, 2024

What are your plans with the pipeline logic? #4125 does not seem to make use of the conditional rollback

The new condition for the rollback ended up in this pull request, in HTC_RxInit().

I do not understand what the meaning of a "pipeline at the end of the workspace" would be?

I can't find a reference to the text you quoted.

@dridi
Copy link
Member Author

dridi commented Jul 8, 2024

What are your plans with the pipeline logic?

I forgot to answer this specific question.

Fetching trailers is done with the same building blocks that fetch headers. And they feed from pipelined bytes before fetching more. Your observation is on point, the pull request currently doesn't leave any room for pipelining, because chunked framing is still fetched one byte at a time.

Moving away from this suboptimal (euphemism) mode of operations is definitely a goal. I opted for status quo, because that reduced the amount of changes for the first step.

@nigoroll
Copy link
Member

nigoroll commented Jul 8, 2024

bugwash: I still do not understand how the first patch is going to be used later, because #4125 does not currently use it, IIUC. But I do not want to hold things up.

@dridi
Copy link
Member Author

dridi commented Jul 8, 2024

It prevents HTC_RxInit() from rolling back a req workspace for trailers.

bugwash: merge and rebase #4125 onto trunk.

@dridi dridi merged commit 0c8448e into varnishcache:master Jul 8, 2024
11 checks passed
@dridi dridi deleted the ws_trailers_prep branch July 8, 2024 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants