-
Notifications
You must be signed in to change notification settings - Fork 217
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
Replace some Sprintfs with string concatenation #1345
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this is worth it, but made a comment where maybe simple string concatenation can be good enough for you
internal/internal_workflow.go
Outdated
@@ -751,7 +759,7 @@ func (c *channelImpl) Receive(ctx Context, valuePtr interface{}) (more bool) { | |||
} | |||
break // Corrupt signal. Drop and reset process. | |||
} | |||
state.yield(fmt.Sprintf("blocked on %s.Receive", c.name)) | |||
state.yield(printfString{fmt: "blocked on %s.Receive", arg1: c.name}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
state.yield(printfString{fmt: "blocked on %s.Receive", arg1: c.name}) | |
state.yield("blocked on "+c.name+".Receive") |
Can you microbenchmark this approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah, that provides a basically identical improvement, and is simpler. I'll switch to that. I might have hoped the compiler could optimize this but I guess Go doesn't go for stuff like that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you running with PGO during your benchmark?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't think I've ever built a server with PGO. That's an interesting angle to try. I couldn't find any indication that the compiler would partially-evaluate a fmt.Sprintf even with PGO though.
(In general we don't care much about cpu usage since the server is generally io-bound, but in this situation it was replaying a ton of schedule workflows and going over its cpu reservation, so I looked for some quick improvements there.)
What was changed
Replace Sprintf with concatenation in a few places.
Why?
This was visible in a profile of some worker pods doing heavy replays so I thought I'd try to improve it.
I created a benchmark out of the schedules replay test:
(best of 10 runs for each, after had less variance also)
So a 1-2% improvement in cpu and a tiny bit less memory.
Checklist
existing unit tests, plus simple benchmark