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

Remove AOF commit blocking on State Invariant or Aof Independent Commands #511

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Remove AOF commit blocking on State Invariant or Aof Independent Commands #511

wants to merge 16 commits into from

Conversation

hamdaankhalid
Copy link

@hamdaankhalid hamdaankhalid commented Jul 5, 2024 •
edited
Loading

This PR removes the need for commands of a certain type to wait for commits on the AOF log before sending response back to client.

The commands that qualify for no needing to be blocked on AOF are commands that are either:

  1. State Invariant Commands that return static data (PING, ECHO, etc)
  2. Commands that are a part of a system that does not depend on the AOF logging at all. (ACL, Latency, etc)

These commands do not need to be blocked on WaitForCommit to complete if there are other RespServerSessions concurrently enqueueing commits to the shared AOF across the sessions.

RespParseStressTest Bench marking results left hand side is my branch, and right handside is main branch:

image
@hamdaankhalid
Copy link
Author

@hamdaankhalid please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree company="Microsoft"
@hamdaankhalid hamdaankhalid force-pushed the hamdaankhalid/optimize_read_paths_aof_block branch 4 times, most recently from 2f5dbf1 to f18ef82 Compare July 5, 2024 04:30
@hamdaankhalid hamdaankhalid changed the title Remove AOF commit blocking on read commands Jul 5, 2024
@hamdaankhalid hamdaankhalid marked this pull request as draft July 5, 2024 04:43
@hamdaankhalid hamdaankhalid force-pushed the hamdaankhalid/optimize_read_paths_aof_block branch 3 times, most recently from 195c7a5 to 14c5239 Compare July 5, 2024 23:54
@hamdaankhalid hamdaankhalid force-pushed the hamdaankhalid/optimize_read_paths_aof_block branch from 14c5239 to 53e4ca7 Compare July 5, 2024 23:57
@hamdaankhalid hamdaankhalid force-pushed the hamdaankhalid/optimize_read_paths_aof_block branch 2 times, most recently from 2d4bb63 to 0a755a4 Compare July 26, 2024 01:21
@hamdaankhalid hamdaankhalid force-pushed the hamdaankhalid/optimize_read_paths_aof_block branch from 0a755a4 to c89bda1 Compare July 26, 2024 01:24
@hamdaankhalid hamdaankhalid changed the title Remove AOF commit blocking on Non State mutating commands Jul 26, 2024
@hamdaankhalid hamdaankhalid marked this pull request as ready for review July 26, 2024 17:46
@hamdaankhalid hamdaankhalid marked this pull request as draft July 27, 2024 20:59
@hamdaankhalid hamdaankhalid force-pushed the hamdaankhalid/optimize_read_paths_aof_block branch from b4838e4 to c977ceb Compare July 28, 2024 17:54
@hamdaankhalid hamdaankhalid force-pushed the hamdaankhalid/optimize_read_paths_aof_block branch 4 times, most recently from 405d2fe to ecf2862 Compare July 31, 2024 00:51
@hamdaankhalid hamdaankhalid marked this pull request as ready for review July 31, 2024 00:53
@hamdaankhalid hamdaankhalid force-pushed the hamdaankhalid/optimize_read_paths_aof_block branch 3 times, most recently from a79f58a to b8de7eb Compare July 31, 2024 02:19
@hamdaankhalid hamdaankhalid force-pushed the hamdaankhalid/optimize_read_paths_aof_block branch from b8de7eb to f352ced Compare July 31, 2024 02:52
TedHartMS
TedHartMS requested changes Jul 31, 2024
View reviewed changes
libs/server/Resp/Parser/RespCommand.cs Outdated Show resolved Hide resolved
libs/server/Resp/RespServerSession.cs Outdated Show resolved Hide resolved
@hamdaankhalid hamdaankhalid changed the title Remove AOF commit blocking on State Invairant or Aof Independent Commands Jul 31, 2024
badrishc
badrishc reviewed Jul 31, 2024
View reviewed changes
libs/server/Resp/RespServerSession.cs Outdated
@@ -299,6 +312,9 @@ public override int TryConsumeMessages(byte* reqBuffer, int bytesReceived)
}
finally
{
// reset the session's flag for AOF blocking to default value after processing all commands
waitForAofBlocking = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this to https://github.com/microsoft/garnet/pull/511/files#diff-b42721ea9aeab99ac7f5177323136d172b8d0c441c03df3f3634f96aa1774ac6L907
Copy link
Author

Choose a reason for hiding this comment

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

inorder to make the session flag not be reset for the scenario where a single commands response may be batched over the network buffer, I made a slight change to Send method to pass context in case it is being called by the WriteDirectLarge method, but I moved the code as suggested
badrishc
badrishc reviewed Jul 31, 2024
View reviewed changes
AOF/aof.log.0 Outdated Show resolved Hide resolved
badrishc
badrishc reviewed Jul 31, 2024
View reviewed changes
libs/server/Resp/RespServerSession.cs
networkSender.GetResponseObject();
dcurr = networkSender.GetResponseObjectHead();
dend = networkSender.GetResponseObjectTail();
}
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private void Send(byte* d)
private void Send(byte* d, bool isChunked = false)
Copy link
Contributor

@badrishc badrishc Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We call SendAndReset in many places after partial command processing, and those are not caught by this code change. It seems it can be made correct by moving the setting to false of waitForAofBlocking, back to the finally in TryConsumeMessages.

But unfortunately it is a bit conservative, because we might have 100 commands in the batch, the first command made waitForAofBlocking true and we finished esnding its result on network after blocking AOF flush, yet the bool will continue to stay true and block the AOF flush for all the remaining 99 commands even if they did not need to block.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants
@hamdaankhalid @TedHartMS @badrishc @pradeepyadavmsft @hamdaankhalidmsft