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

Rotate02 #435

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

Rotate02 #435

wants to merge 3 commits into from

Conversation

cliffjansen
Copy link
Contributor

the first commit does fewer buffer defrag/rotates in the case of outgoing deliveries.

the second takes advantage of code of first to promote more fairness in scheduling

third makes many calls to buffer defrag more performant. The first commit ensures that an outgoing delivery is never wrapped, so this optimization always applies. The special case of cj-sender runs > 10x faster, but streaming messages should in aggregate enjoy a performance boost or reduced cpu load.

Copy link
Member

@astitcher astitcher left a comment

Choose a reason for hiding this comment

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

On the whole I really dislike this change: it complicates the code in a way that makes it harder to understand and doesn't introduce any abstractions to ease comprehension in any way.
I think we should only introduce this change if it gives us an observable performance improvement in real use cases.

Comment on lines +239 to +242
if (pni_buffer_wrapped(buf))
pn_buffer_rotate(buf, buf->start);
else
memmove(buf->bytes, buf->bytes + buf->start, buf->size);
Copy link
Member

Choose a reason for hiding this comment

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

I think this logic is actually part of the _rotate function and should be there.

Also our code formatting has the braces explicitly present for this format of if:

if (...) {
  ...
} else {
  ...
}'''

@astitcher
Copy link
Member

There are 2 use cases for pn_buffer_t:

  1. As an intermediate buffer in the output logic chain. In this usage I'm pretty sure that we can stop automatically defragging the buffer and only do it on a specific request. In this world the pn_buffer_bytes API can obviously only return one contiguous segment of the buffer, but there can only be 2 of them so instead of using pn_buffer_bytes once to get the whole buffer using it twice in succession will get both segments if the buffer has wrapped.
  2. As a hacky way to manage a grow only buffer that is never in practice defragged anyway - I think all uses of ```pn_buffer_memory`` are like this. We should just introduce a new buffer api for these cases.

@cliffjansen
Copy link
Contributor Author

Thanks for taking a look.

I will try to rework this as suggested.

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.

2 participants