Add fix to reduce memory and cpu consumption #4690
Conversation
By reducing how much memory it uses as well as how often it is allocated, we should also reduce the cpu usage since GC will have an easier time cleaning up.
ankur22
requested review from
oleiade and
codebien
and removed request for
a team
|
@ankur22 I see this pull request being marked as draft, but there was a review request at the same time? Can you clarify your expectation, please? In addition, I don't see a pool used here, it sounds you probably want to introduce one, to maximize the effort. Did you consider it? |
@codebien and @oleiade sorry for the noise! This is indeed a draft PR... i'm not sure how to open one without it auto assigning reviewers.
Good shout, do you have any material you can point me to so that I can understand the implementation of a pool of memory? EDIT: I think you're referring to |
@ankur22 If you open it directly as a draft, then reviewers are not assigned. They will be when you turn it into a
Yes, my proposal was around using |
| // Clone the bytes from the recvBuffer to buf before unmarshaling | ||
| // the message. Without this, the unmarshaling will fail as it | ||
| // reuses the buf as the underlying bytes buffer. | ||
| buf := bytes.Clone(buffer.Bytes()) |
There was a problem hiding this comment.
@codebien thanks for the sync.Pool suggestion. I think there's some promise in going down that root. However, it's not going to be a straight forward change. Since the browser module works with events, it's not straight forward to keep track of all the events and have them put the buffer back in the pool. We'd need to refactor a lot of the codebase to be able to put the underlying buffer back in the pool, either by keeping track of the data or by allowing the handlers of the event to put the buffer back into the pool 😭
What?
Using a single
bytes.Bufferacross the lifespan of the connection when reading from the websocket.Why?
By reducing how much memory it uses as well as how often it is allocated, we should also reduce the cpu usage since GC will have an easier time cleaning up.
Checklist
make check) and all pass.Checklist: Documentation (only for k6 maintainers and if relevant)
Please do not merge this PR until the following items are filled out.
Related PR(s)/Issue(s)