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

[Perf Improvement] Random reads should re-use previous reader even if existing reader can serve the request #1683

Merged
merged 3 commits into from Feb 8, 2024

Conversation

ashmeenkaur
Copy link
Collaborator

@ashmeenkaur ashmeenkaur commented Feb 6, 2024

Description

Random reads were not working as intended; they did not reuse the existing reader, even when existing reader had the data to serve the request.

This issue occurred because we were using the Read method (instead of ReadFull). According to the documentation, this method does not wait for the entire data to be available, preventing the reader's start offset from being moved by the required number of bytes.

Fix: We should use io.ReadFull() instead.

Before this change:

time="06/02/2024 08:34:55.221154" severity=TRACE message="fuse_debug: Op 0x0000016e        connection.go:415] <- ReadFile (inode 2, PID 367041, handle 27, offset 41943040, 16384 bytes)"
time="06/02/2024 08:34:55.221364" severity=TRACE message="gcs: Req             0x2c: -> Read(\"1\", [31457280, 32505856)) (330.506132ms): OK"
time="06/02/2024 08:34:55.221400" severity=TRACE message="gcs: Req             0x2d: <- Read(\"1\", [41943040, 42991616))"
time="06/02/2024 08:34:55.576079" severity=TRACE message="fuse_debug: Op 0x0000016e        connection.go:497] -> OK ()"
time="06/02/2024 08:34:55.576375" severity=TRACE message="fuse_debug: Op 0x00000170        connection.go:415] <- ReadFile (inode 2, PID 367041, handle 27, offset 41959424, 32768 bytes)"
time="06/02/2024 08:34:55.751697" severity=TRACE message="fuse_debug: Op 0x00000170        connection.go:497] -> OK ()"
time="06/02/2024 08:34:55.751938" severity=TRACE message="fuse_debug: Op 0x00000172        connection.go:415] <- ReadFile (inode 2, PID 367041, handle 27, offset 42012672, 20480 bytes)"
time="06/02/2024 08:34:55.752120" severity=TRACE message="gcs: Req             0x2d: -> Read(\"1\", [41943040, 42991616)) (530.710831ms): OK"
time="06/02/2024 08:34:55.752174" severity=TRACE message="gcs: Req             0x2e: <- Read(\"1\", [42012672, 43061248))"
time="06/02/2024 08:34:56.002057" severity=TRACE message="fuse_debug: Op 0x00000172        connection.go:497] -> OK ()"

After this change:

time="06/02/2024 09:58:56.900148" severity=TRACE message="fuse_debug: Op 0x0000015a        connection.go:415] <- ReadFile (inode 2, PID 421837, handle 26, offset 41943040, 16384 bytes)"
time="06/02/2024 09:58:56.900362" severity=TRACE message="gcs: Req             0x29: -> Read(\"1\", [31457280, 32505856)) (256.173234ms): OK"
time="06/02/2024 09:58:56.900407" severity=TRACE message="gcs: Req             0x2a: <- Read(\"1\", [41943040, 42991616))"
time="06/02/2024 09:58:57.124098" severity=TRACE message="fuse_debug: Op 0x0000015a        connection.go:497] -> OK ()"
time="06/02/2024 09:58:57.124378" severity=TRACE message="fuse_debug: Op 0x0000015c        connection.go:415] <- ReadFile (inode 2, PID 421837, handle 26, offset 41959424, 32768 bytes)"
time="06/02/2024 09:58:57.284816" severity=TRACE message="fuse_debug: Op 0x0000015c        connection.go:497] -> OK ()"
time="06/02/2024 09:58:57.285058" severity=TRACE message="fuse_debug: Op 0x0000015e        connection.go:415] <- ReadFile (inode 2, PID 421837, handle 26, offset 42012672, 20480 bytes)"
time="06/02/2024 09:58:57.445157" severity=TRACE message="fuse_debug: Op 0x0000015e        connection.go:497] -> OK ()"

Link to the issue in case of a bug fix.

Internal bug b/322770463

Testing details

  1. Manual - Manually verified from logs
  2. Unit tests - added
  3. Integration tests - Ran via KOKORO
  4. Perf tests -
    9yBG5pMAZAVgqA3

@ashmeenkaur ashmeenkaur added the execute-integration-tests Run only integration tests label Feb 6, 2024
@ashmeenkaur ashmeenkaur added execute-perf-test Execute performance test in PR and removed execute-integration-tests Run only integration tests labels Feb 6, 2024
@ashmeenkaur ashmeenkaur changed the title [Fix] Random reads won't re-use previous reader even if within the range of 8MiBs [Fix] Random reads won't re-use previous reader even if existing reader can serve the request Feb 6, 2024
@ashmeenkaur ashmeenkaur marked this pull request as ready for review February 7, 2024 04:17
@ashmeenkaur ashmeenkaur removed the execute-perf-test Execute performance test in PR label Feb 8, 2024
@ashmeenkaur ashmeenkaur merged commit 342c39b into master Feb 8, 2024
8 checks passed
@ashmeenkaur ashmeenkaur deleted the fix_random_read_issue branch February 8, 2024 06:50
@ashmeenkaur ashmeenkaur changed the title [Fix] Random reads won't re-use previous reader even if existing reader can serve the request [Perf Improvement] Random reads won't re-use previous reader even if existing reader can serve the request Feb 29, 2024
@ashmeenkaur ashmeenkaur changed the title [Perf Improvement] Random reads won't re-use previous reader even if existing reader can serve the request [Perf Improvement] Random readsshould re-use previous reader even if existing reader can serve the request Feb 29, 2024
@ashmeenkaur ashmeenkaur changed the title [Perf Improvement] Random readsshould re-use previous reader even if existing reader can serve the request [Perf Improvement] Random reads should re-use previous reader even if existing reader can serve the request Feb 29, 2024
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.

None yet

2 participants