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.

Sign up for GitHub

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

Jump to bottom

disk-buffer: add qout-like memory queue to reliable disk-buffer #3754

Merged
merged 15 commits into from Aug 18, 2021
Merged

disk-buffer: add qout-like memory queue to reliable disk-buffer #3754

merged 15 commits into from Aug 18, 2021

Conversation

alltilla
Copy link
Collaborator

@alltilla alltilla commented Aug 11, 2021 •
edited

This PR adds a fast-path memory queue implementation to the reliable disk-buffer, which can save time, when messages are coming in with a smaller rate, similarly to the qout memory queue in the non-reliable disk-buffer.

This queue still guarantees, that all the received messages are written to the disk as soon as possible.

Also added qout-size() option to reliable disk-buffer, which limits the number of messages in the new memory queue.

The idea behind this improvement, is that if there is space in qout, additionally to storing the serialized message on the disk, we store the message in memory, too. When we arrive to that message on the disk, we do not read it from the disk, and we do not deserialize it, but use the one, stored in the memory, and skip the one on the disk.

Performance gain:
On my computer, I tested different input message rates, and checked, which is the highest, that the disk-buffer can follow with the queued counter staying near 0:

  • master: ~60k msg/sec
  • This PR with qout-size(100000): ~80k msg/sec

Signed-off-by: Attila Szakacs attila.szakacs@oneidentity.com

@alltilla alltilla added the user-visible-feature User visible feature label Aug 11, 2021
@kira-syslogng
Copy link
Contributor

Build FAILURE

@alltilla
Copy link
Collaborator Author

@kira-syslogng test this please test branch=diskq-perf-reliable-qout;

@alltilla
Copy link
Collaborator Author

@kira-syslogng do stresstest

@kira-syslogng
Copy link
Contributor

Build SUCCESS

@kira-syslogng
Copy link
Contributor

Kira-stress-test: Build SUCCESS

@alltilla
Copy link
Collaborator Author

Rebased to master.

@alltilla
Copy link
Collaborator Author

@kira-syslogng do stresstest

@alltilla
Copy link
Collaborator Author

@kira-syslogng test this please test branch=diskq-perf-reliable-qout;

@alltilla
Copy link
Collaborator Author

Cherry picked MrAnno@d37895a

@alltilla
Copy link
Collaborator Author

@kira-syslogng test this please test branch=diskq-perf-reliable-qout;

@alltilla
Copy link
Collaborator Author

@kira-syslogng do stresstest

@kira-syslogng
Copy link
Contributor

Build SUCCESS

@kira-syslogng
Copy link
Contributor

Kira-stress-test: Build SUCCESS

@MrAnno MrAnno self-requested a review August 12, 2021 14:19
MrAnno
MrAnno reviewed Aug 13, 2021
View reviewed changes
modules/diskq/logqueue-disk-reliable.c Show resolved Hide resolved
modules/diskq/logqueue-disk-reliable.c Outdated Show resolved Hide resolved
modules/diskq/logqueue-disk-reliable.c Outdated Show resolved Hide resolved
modules/diskq/logqueue-disk-reliable.c Show resolved Hide resolved
modules/diskq/logqueue-disk-reliable.c Outdated Show resolved Hide resolved
news/feature-3754.md Outdated Show resolved Hide resolved
@alltilla
Copy link
Collaborator Author

@kira-syslogng test this please test branch=diskq-perf-reliable-qout;

@alltilla
Copy link
Collaborator Author

@kira-syslogng do stresstest

@kira-syslogng
Copy link
Contributor

Build FAILURE

@alltilla
Copy link
Collaborator Author

@kira-syslogng test this please test branch=diskq-perf-reliable-qout;

@alltilla
Copy link
Collaborator Author

@kira-syslogng do stresstest

@alltilla alltilla mentioned this pull request Aug 13, 2021
Merged
@alltilla
logqueue-disk-reliable: factor out _pop_from_memory_queue_head()
82bc615 
Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>
@alltilla
logqueue-disk-reliable: factor out _peek_next_msg_position()
b5ebaa1 
Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>
@alltilla
logqueue-disk-reliable: allocate and free position in memory_queue fu…
89b2447 
…nctions

Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>
@alltilla
logqueue-disk-reliable: refactor acking and refing in push_tail()
f424dee 
Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>
@alltilla
logqueue-disk-reliable: factor out _is_next_message_in_qreliable()
5e33a3a 
Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>
@alltilla
logqueue-disk-reliable: early return in pop_head()
98df50c 
Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>
@alltilla
logqueue-disk-reliable: early return in pop_head()
26769a9 
Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>
@alltilla
logqueue-disk-reliable: add skeleton of qout memory queue
8ce1dbe 
This memory queue will work similar to the one in non-reliable, with the
difference, that we still store the messages on the disk, additionally
to storing it in the memory queue.

The main usage of qout in the reliable disk buffer is to have some msgs
stored in memory, so when we get to the point in the qdisk, where we
also have that msg stored in the memory, we just skip the one on the
disk, and use the one in the memory, saving one message read from the
disk, and one deserialization.

The first difference between qout and qreliable is that we ack the msg,
because we do not want to trigger the flow-control.
The other difference is, that we do not move from qout to qbacklog,
we let the qdisk's backlog handling handle the situation.

Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>
@alltilla
logqueue-disk-reliable: use qout-size() from grammar
8f2973c 
Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>
@alltilla
logqueue-disk-reliable: implement qout
2fdea88 
See: logqueue-disk-reliable: add skeleton of qout memory queue

Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>
@alltilla
logqueue-disk: set ack_needed to FALSE in read_message
3432ffc 
Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>
@MrAnno
Copy link
Collaborator

MrAnno commented Aug 15, 2021

@kira-syslogng test this please test branch=diskq-perf-reliable-qout;

@kira-syslogng
Copy link
Contributor

Build SUCCESS

Kokan
Kokan previously approved these changes Aug 16, 2021
View reviewed changes
Copy link
Collaborator

@Kokan Kokan left a comment

Choose a reason for hiding this comment

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

My only comment is regarding news file, and those information should be present in the documentation, so it is fine if not updated here. I'd still prefer to.

news/feature-3754.md
@@ -0,0 +1,10 @@
`disk-buffer`: Added a new option to reliable disk-buffer: `qout-size()`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the default size? Are there limitation (max, min), or should this value be in sync with different parameters ? Mentioning these in the news file could be useful.

Copy link
Collaborator

@MrAnno MrAnno Aug 16, 2021 •
edited

Choose a reason for hiding this comment

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

I've also reviewed this PR and I'm ready to approve it.
Since @alltilla is on vacation, I'm adding the default value (which will be 1000 after merging #3757) to the news entry.
To keep news entries short, I would not mention more.

The minimum value is 0, which is forced by our grammar.
There is no theoretical maximum value, but setting this larger than the disk buffer's size (which is given in bytes, but qout-size counts the number of messages) would not make sense, but it is allowed and it will work correctly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is an extra check in the setter, that enforces the 64 as min value. Maybe allowing 0 wouldn't be a bad thing. What do you think ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh sorry, I misunderstood the comment about 64. Yeah, we should definitely allow 0.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done.

@alltilla @MrAnno
news: add entry for syslog-ng#3754
45d98d1 
Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>
Signed-off-by: László Várady <laszlo.varady@protonmail.com>
@MrAnno
Copy link
Collaborator

MrAnno commented Aug 16, 2021

@kira-syslogng test this please test branch=diskq-perf-reliable-qout;

@kira-syslogng
Copy link
Contributor

Build SUCCESS

@MrAnno
disk-buffer: allow <64 values for qout-size()
28465d6 
This way, qout can be completely disabled for the reliable and the
non-reliable disk buffer.

Signed-off-by: László Várady <laszlo.varady@protonmail.com>
@kira-syslogng
Copy link
Contributor

Build FAILURE

@MrAnno
Copy link
Collaborator

MrAnno commented Aug 17, 2021

@kira-syslogng test this please test branch=diskq-perf-reliable-qout;

MrAnno pushed a commit to MrAnno/syslog-ng that referenced this pull request Aug 17, 2021
@alltilla @MrAnno
news: add entry for syslog-ng#3754
555706a 
Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>
Signed-off-by: László Várady <laszlo.varady@protonmail.com>
@kira-syslogng
Copy link
Contributor

Build SUCCESS

@Kokan Kokan added this to the syslog-ng-3.34 milestone Aug 18, 2021
@MrAnno MrAnno merged commit 8f6fa9e into syslog-ng:master Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kokan Kokan Kokan approved these changes

@MrAnno MrAnno MrAnno approved these changes

Assignees
No one assigned
Labels
user-visible-feature User visible feature
Projects
None yet
Milestone
syslog-ng-3.34
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@alltilla @kira-syslogng @MrAnno @Kokan