avoid unnecessary buffer copies on opennext.js response #1010
Conversation
🦋 Changeset detectedLatest commit: a8463c7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
| // Avoid keeping chunks around when the `StreamCreator` supports it to save memory | ||
| this._chunks.push(buffer); | ||
| // Invalidate cached body when new chunks are added | ||
| this._cachedBody = undefined; |
There was a problem hiding this comment.
This can be a bit problematic for memory. This means that we're persistently doubling the amount of memory consumed. Sure it means we eliminate multiple potential copies but at the cost of definitely holding more memory.
Instead, if we're going to cache this, we should replace this._chunks with the combined buffer so that the separate chunks can be reclaimed.
I think a better approach is to see if we can eliminate this entirely, and instead rely on regular stream buffering.
Ultimately I'm not sure if this is the right change.
| get finished() { | ||
| return this.responseStream | ||
| ? this.responseStream?.writableFinished | ||
| ? this.responseStream.writableFinished |
There was a problem hiding this comment.
@anonrig ... just a general note. This looks like a drive-by change unrelated to the actual unnecessary buffer copies. For additional changes like this, can you put these into separate commits moving forward? Makes it a bit difficult to separate what is or is not relevant to the actual allocation/copy improvements.
3 participants
Several optimizations but I think the most impactful one is to avoid having an unnecessary copy everytime getBody() is called.