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

Removes file sync syscall for compaction. #3693

Merged
merged 1 commit into from
May 6, 2021

Conversation

cyriltovena
Copy link
Contributor

@cyriltovena cyriltovena commented May 6, 2021

Because we mostly download, read and upload but also because all those operations are idempotent we don't
need to try to outsmart the OS by forcefully syncing the file to disk. The OS will use cache on files and we'll most likely not suffer from high disk io.

This was a conclusion I made after some digging of why the operation of compaction+retention was a bit slow sometimes. (+5s)

I've tried the PR in dev and the whole process execute now below 40s whereas because it was 4min.

strace before:

% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
 75.75   12.333637        2122      5812       561 futex
 10.81    1.760698         242      7267           nanosleep
  7.69    1.252697          88     14287           epoll_pwait
  2.86    0.464888          22     21608           write
  1.08    0.175313       19479         9           fsync                <----------
  1.06    0.172173          15     11249      4334 read
  0.40    0.064422        2147        30        15 unlinkat
  0.15    0.023945          19      1246           madvise
  0.09    0.015288        1529        10           munmap
  0.04    0.006098          23       263           tgkill
  0.03    0.004134          16       263           getpid

strace after:

% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
 77.01   58.085485        4599     12631       840 futex
  9.81    7.395901         230     32145           nanosleep
  8.34    6.287953         175     35882           epoll_pwait
  3.33    2.512512          18    141141           write
  0.79    0.596351          16     37911     13536 read
  0.51    0.382579        3985        96        48 unlinkat
  0.08    0.058465        1720        34           munmap
  0.04    0.031904          26      1219           tgkill
  0.03    0.023962          22      1082           madvise
  0.02    0.013831          11      1219           getpid
  0.01    0.011042           9      1219        12 rt_sigreturn
  0.01    0.005348         111        48           fdatasync
  0.01    0.003785          36       105           openat
  0.00    0.003603           8       435           pwrite64
  0.00    0.003005          13       229       200 epoll_ctl
  0.00    0.001828          19        96        34 newfstatat

As you can see the fsync and write are less intensive after.

image

I did not changed the write and read path though, I did not investigate if this was needed.

Signed-off-by: Cyril Tovena cyril.tovena@gmail.com

Because we mostly download, read and upload but also  because all those operations are idempotent we don't
need to try to outsmart the OS by forcefully syncing the file to disk. The OS will use cache on files and we'll most likely not suffer from high disk io.

This was a conclusion I made after some digging of why the operation of compaction+retention was a bit slow sometimes. (+5s)

I've tried the PR in dev and the whole process execute now below 40s whereas because it was 4min.

strace before:

```
% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
 75.75   12.333637        2122      5812       561 futex
 10.81    1.760698         242      7267           nanosleep
  7.69    1.252697          88     14287           epoll_pwait
  2.86    0.464888          22     21608           write
  1.08    0.175313       19479         9           fsync                <----------
  1.06    0.172173          15     11249      4334 read
  0.40    0.064422        2147        30        15 unlinkat
  0.15    0.023945          19      1246           madvise
  0.09    0.015288        1529        10           munmap
  0.04    0.006098          23       263           tgkill
  0.03    0.004134          16       263           getpid
```

strace after:

```
% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
 77.01   58.085485        4599     12631       840 futex
  9.81    7.395901         230     32145           nanosleep
  8.34    6.287953         175     35882           epoll_pwait
  3.33    2.512512          18    141141           write
  0.79    0.596351          16     37911     13536 read
  0.51    0.382579        3985        96        48 unlinkat
  0.08    0.058465        1720        34           munmap
  0.04    0.031904          26      1219           tgkill
  0.03    0.023962          22      1082           madvise
  0.02    0.013831          11      1219           getpid
  0.01    0.011042           9      1219        12 rt_sigreturn
  0.01    0.005348         111        48           fdatasync
  0.01    0.003785          36       105           openat
  0.00    0.003603           8       435           pwrite64
  0.00    0.003005          13       229       200 epoll_ctl
  0.00    0.001828          19        96        34 newfstatat
```

As you can see the fsync and write are less intensive after.

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

LGTM

@cyriltovena cyriltovena merged commit 14fa7d8 into grafana:main May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants