Fix alignment in blockWriteStream. #82
Conversation
|
BTW I needed this in order for performance to be adequate. Uploading a 8GB file was taking > 1hr. After this change it took 90 seconds. |
|
I added an issue #84 |
|
|
||
| packet := bws.makePacket() | ||
| // if we are totally aligned, then the packet should be outboundPacketSize. | ||
| if len(packet.data) != outboundPacketSize { |
There was a problem hiding this comment.
Can you use assert.NotEqual here (and assert.Equal below)?
| func TestPacketAlignment(t *testing.T) { | ||
| // Verify that alignment is correctly handled in blockWriteStream. | ||
| bws := &blockWriteStream{} | ||
| // Write some data so we can test making packets. |
There was a problem hiding this comment.
You can get rid of these two comments, they're just noisy.
| @@ -173,7 +173,7 @@ func (s *blockWriteStream) makePacket() outboundPacket { | |||
| // gets unhappy unless we first align to a chunk boundary with a small packet. | |||
| // Otherwise it yells at us with "a partial chunk must be sent in an | |||
| // individual packet" or just complains about a corrupted block. | |||
| alignment := outboundChunkSize - (int(s.offset) % outboundChunkSize) | |||
| alignment := (outboundChunkSize - (int(s.offset) % outboundChunkSize)) % outboundChunkSize | |||
There was a problem hiding this comment.
I think I see the problem now, ugh. An extra % just makes this line more confusing, though. I think something like this would scan better:
alignment := int(s.offset) % outboundChunkSize
if alignment != 0 && packetLength > outboundChunkSize {
packetLength = outboundChunkSize - alignment
}
What do you think?
|
Gah, thanks for catching this. Super terrible. I've got some nitpicks on the fix and the test changes, then I'll merge. |
|
I merged your fix in cdda132. Thanks again! |
|
Ah sorry for not addressing those nitpicks, I've been super busy! Thanks for merging my fix. |
There was a bug that caused packet size to always equal chunkSize (512 bytes). I discovered this when debugging performance with
perf trace.Fixed this by taking
alignment = (chunkSize - (offset % chunkSize)) % chunkSize, which will be 0 when the offset is aligned.Added a simple test to verify this.