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

Preallocate buffer size when reading data #239

Merged
merged 6 commits into from Feb 15, 2022
Merged

Preallocate buffer size when reading data #239

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
86dc983
Preallocate buffer size when reading data
pfreixes Jan 17, 2022
a9edd80
Change the threshold for 256KB
pfreixes Jan 19, 2022
99714d5
Add description about 256KB rationale
pfreixes Jan 20, 2022
dbd7d13
Fix tests and use Buffer.alloc
pfreixes Jan 20, 2022
bab41d9
Modify test timeout that checks performance of the chunk parser
pfreixes Jan 20, 2022
ef3a4ee
Less aggressive timeout
pfreixes Jan 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 26 additions & 7 deletions lib/eventsource.js
View file
Open in desktop
Expand Up @@ -15,6 +15,8 @@ var colon = 58
var space = 32
var lineFeed = 10
var carriageReturn = 13
// 1MB
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could you specify why you landed at 1MB?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, TBH I piked up this number without data and expecting that would provide enough head room without impacting to much on the free memory.

Ive just tried different configurations and seems that the sweet spot is 256KBs, beyond this there is no gain in the performance.

2KB 9902ms
4KB 8192ms
8KB 5115ms
16KB 2529ms
32KB 1859ms
64KB 911ms
128KB 801ms
256KB 539ms
512KB 562ms
1MB 565ms

I will modify the 1MB max threshold to 256KBs

Copy link
Contributor

Choose a reason for hiding this comment

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

Great! Would you mind just adding a comment noting this? You could even link to this if you want to. I just think we should make it clear to someone reading the code how we picked what could be an arbitrary value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

var maxBufferAheadAllocation = 1024*1024

function hasBom (buf) {
return bom.every(function (charCode, index) {
Expand Down Expand Up @@ -179,19 +181,35 @@ function EventSource (url, eventSourceInitDict) {

// text/event-stream parser adapted from webkit's
// Source/WebCore/page/EventSource.cpp
var isFirst = true
var buf
var startingPos = 0
var startingFieldLength = -1
var newBufferSize = 0
var bytesUsed = 0

res.on('data', function (chunk) {
buf = buf ? Buffer.concat([buf, chunk]) : chunk
if (isFirst && hasBom(buf)) {
buf = buf.slice(bom.length)
if (!buf) {
buf = chunk
if (hasBom(buf)) {
buf = buf.slice(bom.length)
}
} else {
if (chunk.length > buf.length - bytesUsed) {
newBufferSize = (buf.length * 2) + chunk.length
if (newBufferSize > maxBufferAheadAllocation) {
newBufferSize = buf.length + chunk.length + maxBufferAheadAllocation
}
newBuffer = new Buffer(newBufferSize)
buf.copy(newBuffer, 0, 0, bytesUsed)
buf = newBuffer
}
chunk.copy(buf, bytesUsed)
}

isFirst = false
bytesUsed += chunk.length

var pos = 0
var length = buf.length
var length = bytesUsed

while (pos < length) {
if (discardTrailingNewline) {
Expand Down Expand Up @@ -236,7 +254,8 @@ function EventSource (url, eventSourceInitDict) {
if (pos === length) {
buf = void 0
} else if (pos > 0) {
buf = buf.slice(pos)
buf = buf.slice(pos, bytesUsed)
bytesUsed = buf.length
}
})
})
Expand Down