disk-buffer: trivial performance-related fixes and refactor #3743
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Build SUCCESS |
|
@kira-syslogng do stresstest |
|
Kira-stress-test: Build SUCCESS |
Kokan
reviewed
Jul 28, 2021
szemere
reviewed
Jul 28, 2021
szemere
reviewed
Jul 28, 2021
| @@ -427,6 +427,16 @@ _is_trying_to_read_from_unwritten_space(QDisk *self) | |||
| return self->hdr->read_head == self->hdr->write_head; | |||
| } | |||
|
|
|||
| static gssize | |||
| _read_record_length_from_disk(QDisk *self, guint32 *record_length) | |||
There was a problem hiding this comment.
optional: I was about to comment the lack of this (6aca8bf3961ae83a2d68ebf80bbcd0ab0b0ca397) refactor here, when I found it. Squash them together?
alltilla
force-pushed
the
diskq-perf-low-hanging-fixes
branch
from
628e8aa
to
8ea0672
Compare
|
Build FAILURE |
gaborznagy
reviewed
Jul 29, 2021
alltilla
force-pushed
the
diskq-perf-low-hanging-fixes
branch
from
8ea0672
to
7967f90
Compare
|
Build SUCCESS |
Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>
Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>
Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>
Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>
Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>
Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>
…d move
We can use that value, because they are the same.
From glib/glib/gstring.c:
```
GString *
g_string_set_size (GString *string,
gsize len)
{
g_return_val_if_fail (string != NULL, NULL);
if (len >= string->allocated_len)
g_string_maybe_expand (string, len - string->len);
string->len = len;
string->str[len] = 0;
return string;
}
```
Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>
This commit changes the atomicity of changing the value of read_head. It is important, because read_head is on an mmap-ed region, and if a crash occurs, we still want to have a valid disk-buffer. The old could change read_head twice, if the first change pointed to a position, which nedded wrapping, it was then changed again to the beginning of the file (QDISK_RESERVED_SPACE). In this case, if a crash occurred between the two, we did not wrap the read_head, so in the next pop_head after the restart, we might have read from an invalid space. The new code calculates the new read_head position with a local variable and sets read_head once to the valid position. So we can either crash before setting or after setting. If we crash after we already changed the read_head, so everything is fine, if we crashed before, we will try to read the same message again after the restart. Either case we read from a valid position. In conclusion the new logic is safer in case of crash. Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>
Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>
Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>
Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>
Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>
Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>
Signed-off-by: László Várady <laszlo.varady@protonmail.com>
This has no real performance gain, it just makes the responsibilities of LogQueueDisk a bit easier to understand. Signed-off-by: László Várady <laszlo.varady@protonmail.com>
Signed-off-by: László Várady <laszlo.varady@protonmail.com>
Signed-off-by: László Várady <laszlo.varady@protonmail.com>
* concatenate message length and message, then write This results in obvious performance improvement, makes message-writes a bit more atomic, and does not worsen disk-buffer guarantees. Signed-off-by: Balázs Barkó <Balazs.Barko@oneidentity.com>
reason: didn't need to alloc && free GString every time when we want to write message to file Signed-off-by: Balázs Barkó <Balazs.Barko@oneidentity.com>
Signed-off-by: Balázs Barkó <Balazs.Barko@oneidentity.com>
Signed-off-by: László Várady <laszlo.varady@protonmail.com>
MrAnno
force-pushed
the
diskq-perf-low-hanging-fixes
branch
from
7967f90
to
42b6ef3
Compare
|
Build SUCCESS |
gaborznagy
approved these changes
Jul 30, 2021
Kokan
approved these changes
Jul 30, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We are currently profiling and measuring the performance of the
disk-buffer()module in different scenarios in order to identify its bottlenecks and to improve its speed.This PR is the first step in this adventure.
As we got familiar with the code base, we found some trivial performance-related issues, which we've fixed:
pwrite()calls to a single one when writing the serialized message and its lengthThis resulted in a ~20% performance improvement in general.
The PR also contains refactor commits.
For reviewers:
Click to expand!
The reliable disk buffer uses 2 memory queues:
- it's just a technicality, it is only used to kick flow-control in when the free space is less than
mem-buf-size()in the disk-buffer file- messages stored in this queue are also written into the disk-buffer file, so these messages will never be lost
The non-reliable disk buffer uses 3 memory queue segments:
qout-size()- the first
qout-size()number of messages (64 by default) are stored only in the qout memory queue for performance reasons- we only write/read messages to/from disk after the qout queue is full
- this is used for the same reason as qreliable, to kick flow-control in when the disk-buffer file is full
- it stores
mem-buf-length()number of messages after the disk-buffer file is fully saturated