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

fix: Return to read immediately after a successful read. #894

Merged
merged 1 commit into from
Jul 5, 2024

Conversation

jaqx0r
Copy link
Contributor

@jaqx0r jaqx0r commented Jul 5, 2024

Copying the filestream, now pipe, dgram, and socket streams return to read
again immediately after a successful read, so that we don't wait.

This is now obviously the problem in #685 and using the bandwidth-delay-product
we can see that a 250ms pause between reads of 4096B and 128KiB matches the
results seen.

Before:

jaq% time ./mtail -logs - -progs examples/rsyncd.mtail < internal/mtail/testdata/rsyncd.log
0.01s user 0.01s system 7% cpu 0.264 total

After:

jaq% time ./mtail -logs - -progs examples/rsyncd.mtail < internal/mtail/testdata/rsyncd.log
0.01s user 0.03s system 102% cpu 0.041 total

Thanks to @rideliner for the hint.

Issue: #685

Base automatically changed from logstream-shutdown to main July 5, 2024 06:31
@jaqx0r jaqx0r force-pushed the read-after-read branch 2 times, most recently from a80432a to b01ee8c Compare July 5, 2024 06:32
@jaqx0r jaqx0r enabled auto-merge July 5, 2024 06:33
Copy link
Contributor

github-actions bot commented Jul 5, 2024

Unit Test Results

    1 files     27 suites   8m 40s ⏱️
  648 tests   646 ✅ 1 💤 1 ❌
1 917 runs  1 913 ✅ 3 💤 1 ❌

For more details on these failures, see this check.

Results for commit afd77e4.

♻️ This comment has been updated with latest results.

Copying the filestream, now pipe, dgram, and socket streams return to read
again immediately after a successful read, so that we don't wait.

This is now obvious the problem in #685 and using the bandwidth-delay-product
we can see that a 250ms pause between reads of 4096B and 128KiB matches the
results seen.

Before:
```
jaq% time ./mtail -logs - -progs examples/rsyncd.mtail < internal/mtail/testdata/rsyncd.log
0.01s user 0.01s system 7% cpu 0.264 total
```

After:
```
jaq% time ./mtail -logs - -progs examples/rsyncd.mtail < internal/mtail/testdata/rsyncd.log
0.01s user 0.03s system 102% cpu 0.041 total
```

Thanks to @rideliner for the hint.
@jaqx0r jaqx0r force-pushed the read-after-read branch from b01ee8c to afd77e4 Compare July 5, 2024 06:50
@jaqx0r jaqx0r added this pull request to the merge queue Jul 5, 2024
Merged via the queue into main with commit 5196546 Jul 5, 2024
22 of 23 checks passed
@jaqx0r jaqx0r deleted the read-after-read branch July 5, 2024 07:11
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.

1 participant