-
Notifications
You must be signed in to change notification settings - Fork 652
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
Make tests work on 32-bit #486
Changes from 6 commits
149b341
6992e1b
dfefac1
4264cf3
3f0541d
d4c40b0
6641f8a
6ab4f11
abd6238
d49bee0
b2fe95b
3246661
1ee4edd
7175fc8
2eb3e6a
fb1a6e1
19a4199
8e8870d
91c38d3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -231,7 +231,7 @@ public struct ByteBuffer { | |
} | ||
|
||
private static func allocateAndPrepareRawMemory(bytes: Capacity, allocator: Allocator) -> UnsafeMutableRawPointer { | ||
let bytes = Int(bytes) | ||
let bytes = Int(bytes) /* Note(hh): this can fail on 32-bit (Int32 vs UInt32 Capacity)) */ | ||
let ptr = allocator.malloc(bytes)! | ||
/* bind the memory so we can assume it elsewhere to be bound to UInt8 */ | ||
ptr.bindMemory(to: UInt8.self, capacity: bytes) | ||
|
@@ -332,7 +332,7 @@ public struct ByteBuffer { | |
self._slice = _ByteBufferSlice(_slice.lowerBound ..< self._storage.capacity) | ||
} | ||
} | ||
assert(self._slice.lowerBound + index + capacity <= self._slice.upperBound) | ||
assert(self._slice.lowerBound + index + capacity <= self._slice.upperBound) // TODO(hh): fails on 32-bit | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove the TODO ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @helje5 why does this fail on 32-bit ? If that fails, the code above doesn't work There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably was a temporary note for myself and I fixed it on the calling side, need to check. |
||
assert(self._slice.lowerBound >= 0, "illegal slice: negative lower bound: \(self._slice.lowerBound)") | ||
assert(self._slice.upperBound <= self._storage.capacity, "illegal slice: upper bound (\(self._slice.upperBound)) exceeds capacity: \(self._storage.capacity)") | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -113,13 +113,19 @@ extension UInt32 { | |
|
||
var n = self | ||
|
||
#if arch(arm) // 32-bit, Raspi/AppleWatch/etc | ||
let max = UInt32(Int32.max) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is maybe nit-picking but we'd like it to be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fine with changing this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not entirely sure about this. You want There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes |
||
#else | ||
let max = UInt32.max | ||
#endif | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change violates the doc comment up above, so we should probably change this. Incidentally, I assume the reason for this change is that "Returns the next power of two unless that would overflow, in which case UInt32.max (on 64-bit systems) or Int32.max (on 32-bit systems) is returned. The returned value is always safe to be cast to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was probably to support the |
||
|
||
n -= 1 | ||
n |= n >> 1 | ||
n |= n >> 2 | ||
n |= n >> 4 | ||
n |= n >> 8 | ||
n |= n >> 16 | ||
if n != .max { | ||
if n != max { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we actually have a test that on 32-bit systems tests that this returns the right value? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no test for this on 64-bit either. /shrug There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that's bad, we should definitely have one |
||
n += 1 | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,14 @@ public typealias IOVector = iovec | |
|
||
/// The maximum number of bytes to write per `writev` call. | ||
static var writevLimitBytes: Int { | ||
return Int(Int32.max) | ||
#if arch(arm) // 32-bit, Raspi/AppleWatch/etc | ||
// Note(hh): This is not a _proper_ fix, but necessary because | ||
// other places extend on that. Should be fine in | ||
// practice on 32-bit platforms. | ||
return Int(Int32.max / 4) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hrm, I don't love this. AFAICT this change works around the fact that we have a test that validates what happens if we attempt to write more than For the test in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, there are more NIO issues wrt that. I.e. ByteBuffer uses UInt32 and nextPoW2 to round up the size, which subsequently fails when casting backt to malloc, which wants an Int. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Doesn't this patch change that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tracked down this specific thing, yes (but stuff kept failing). What I was trying to say is that it probably makes no sense to support that max-addressable-memory edge-case in NIO (generally). If you really want to, I suggest starting out by writing tests that check against Int64.max on 64-bit too. It is not worth it, IMO. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @helje5 I don't understand why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Well, it is in the comment above it (and in my reply above yours). It is not a proper fix, but it is a lot of work to track down all edge cases (for little gain). P.S.: Maybe a lot of that went away when you changed sysMalloc(Int) to sysMalloc(size_t). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @helje5 could we re-check this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @helje5 if you could just re-check that this is still the case. And then just remove the two There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if that's done we can merge I think. |
||
#else | ||
return Int(Int32.max) | ||
#endif | ||
} | ||
|
||
/// The maximum number of `IOVector`s to write per `writev` call. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,14 @@ import NIO | |
|
||
private let maxOneByteSize = 125 | ||
private let maxTwoByteSize = Int(UInt16.max) | ||
private let maxNIOFrameSize = Int(UInt32.max) | ||
#if arch(arm) // 32-bit, Raspi/AppleWatch/etc | ||
// Note(hh): This is not a _proper_ fix, but necessary because | ||
// other places extend on that. Should be fine in | ||
// practice on 32-bit platforms. | ||
private let maxNIOFrameSize = Int(Int32.max / 4) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason not to just set this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. Probably breaks when it travels to the ByteBuffer, which will use UInt32 and then fail. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should fully understand this. I think |
||
#else | ||
private let maxNIOFrameSize = Int(UInt32.max) | ||
#endif | ||
|
||
/// An inbound `ChannelHandler` that serializes structured websocket frames into a byte stream | ||
/// for sending on the network. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1073,22 +1073,37 @@ class ByteBufferTest: XCTestCase { | |
} | ||
|
||
func testAllocationOfReallyBigByteBuffer() throws { | ||
#if !arch(arm) // !32-bit, Raspi/AppleWatch/etc | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we invert this conditional? It'd be nice to see the intended effect of the other branch closer to the top of the test. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume this conditional is intended to be temporary? If it's not, can we remove the tests for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. This fails in a specific ARM memcpy, I hope that we get a fix for that eventually. I raised this in the ARM-community slack. The checks within make the test working until that memcpy error. (maybe that memcpy has additional size or alignment restrictions, no idea yet). |
||
let alloc = ByteBufferAllocator(hookedMalloc: { testAllocationOfReallyBigByteBuffer_mallocHook($0) }, | ||
hookedRealloc: { testAllocationOfReallyBigByteBuffer_reallocHook($0, $1) }, | ||
hookedFree: { testAllocationOfReallyBigByteBuffer_freeHook($0) }, | ||
hookedMemcpy: { testAllocationOfReallyBigByteBuffer_memcpyHook($0, $1, $2) }) | ||
|
||
#if arch(arm) // 32-bit, Raspi/AppleWatch/etc | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. instead of indenting everything, can we just do
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't care if you prefer that, but visually an indent makes sense, IMO. Sometimes people do a 4-indent for regular Swift code and indent the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, you are saying I should return early, not remove the actual indent. But this will produce a Swift compiler warning wrt unreachable code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ahh, ok, then leave this as is. |
||
let reallyBigSize = Int(Int16.max) // well, but Int32 is too big (1GB RAM, no swap) | ||
#else | ||
let reallyBigSize = Int(Int32.max) | ||
#endif | ||
XCTAssertEqual(AllocationExpectationState.begin, testAllocationOfReallyBigByteBuffer_state) | ||
var buf = alloc.buffer(capacity: Int(Int32.max)) | ||
var buf = alloc.buffer(capacity: reallyBigSize) | ||
XCTAssertEqual(AllocationExpectationState.mallocDone, testAllocationOfReallyBigByteBuffer_state) | ||
XCTAssertGreaterThanOrEqual(buf.capacity, Int(Int32.max)) | ||
XCTAssertGreaterThanOrEqual(buf.capacity, reallyBigSize) | ||
|
||
buf.set(bytes: [1], at: 0) | ||
/* now make it expand (will trigger realloc) */ | ||
buf.set(bytes: [1], at: buf.capacity) | ||
|
||
XCTAssertEqual(AllocationExpectationState.reallocDone, testAllocationOfReallyBigByteBuffer_state) | ||
XCTAssertEqual(buf.capacity, Int(UInt32.max)) | ||
#if arch(arm) // 32-bit, Raspi/AppleWatch/etc | ||
// TODO(hh): no idea, but not UInt32.max :-) | ||
XCTAssertGreaterThanOrEqual(buf.capacity, reallyBigSize) | ||
#else | ||
XCTAssertEqual(buf.capacity, Int(UInt32.max)) | ||
#endif | ||
#else | ||
// fails hard on Raspi | ||
XCTAssertTrue(false, "testAllocationOfReallyBigByteBuffer fails on 32-bit Raspi") | ||
#endif | ||
} | ||
|
||
func testWritableBytesAccountsForSlicing() throws { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -169,9 +169,15 @@ public class ChannelTests: XCTestCase { | |
for _ in 0..<bufferSize { | ||
buffer.write(staticString: "a") | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mind removing this whitespace line? |
||
|
||
#if arch(arm) // 32-bit, Raspi/AppleWatch/etc | ||
let lotsOfData = Int(Int32.max / 8) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we use less data here? Is that documented to be the maximum? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but we're only allocating 2MB here, right? We'll write the same buffer over and over again, this should work. The sizes were chosen so it will exhaust both the number of bytes limit (IIRC 2GB) as well as the number of chunks (1024) you're allowed to give to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I see. You just repeatedly write the 2MB buffer. Maybe the overflow happens because of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it'd be awesome if you could validate why it fails. There must be something else wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, see my () comment:
|
||
#else | ||
let lotsOfData = Int(INT32_MAX) // TBD(hh): should that be Int32.max? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please use |
||
#endif | ||
var written = 0 | ||
while written <= Int(INT32_MAX) { | ||
while written <= lotsOfData { | ||
clientChannel.write(NIOAny(buffer), promise: nil) | ||
written += bufferSize | ||
} | ||
|
@@ -220,7 +226,7 @@ public class ChannelTests: XCTestCase { | |
var iovecs: [IOVector] = Array(repeating: iovec(), count: Socket.writevLimitIOVectors + 1) | ||
var managed: [Unmanaged<AnyObject>] = Array(repeating: Unmanaged.passUnretained(o), count: Socket.writevLimitIOVectors + 1) | ||
/* put a canary value at the end */ | ||
iovecs[iovecs.count - 1] = iovec(iov_base: UnsafeMutableRawPointer(bitPattern: 0xdeadbeef)!, iov_len: 0xdeadbeef) | ||
iovecs[iovecs.count - 1] = iovec(iov_base: UnsafeMutableRawPointer(bitPattern: 0x1337beef)!, iov_len: 0x1337beef) | ||
try iovecs.withUnsafeMutableBufferPointer { iovecs in | ||
try managed.withUnsafeMutableBufferPointer { managed in | ||
let pwm = NIO.PendingStreamWritesManager(iovecs: iovecs, storageRefs: managed) | ||
|
@@ -237,8 +243,8 @@ public class ChannelTests: XCTestCase { | |
} | ||
/* assert that the canary values are still okay, we should definitely have never written those */ | ||
XCTAssertEqual(managed.last!.toOpaque(), Unmanaged.passUnretained(o).toOpaque()) | ||
XCTAssertEqual(0xdeadbeef, Int(bitPattern: iovecs.last!.iov_base)) | ||
XCTAssertEqual(0xdeadbeef, iovecs.last!.iov_len) | ||
XCTAssertEqual(0x1337beef, Int(bitPattern: iovecs.last!.iov_base)) | ||
XCTAssertEqual(0x1337beef, iovecs.last!.iov_len) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of interest why does the old value prevent compilation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Int(0xdeadbeef) (which the compiler generates for literals) overflows There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be better to use that other version, just for consistency with ourselves (we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd propose |
||
} | ||
} | ||
|
||
|
@@ -676,8 +682,8 @@ public class ChannelTests: XCTestCase { | |
/// Test that with a few massive buffers, we don't offer more than we should to `writev` if the individual chunks fit. | ||
func testPendingWritesNoMoreThanWritevLimitIsWritten() throws { | ||
let el = EmbeddedEventLoop() | ||
let alloc = ByteBufferAllocator(hookedMalloc: { _ in UnsafeMutableRawPointer(bitPattern: 0xdeadbeef)! }, | ||
hookedRealloc: { _, _ in UnsafeMutableRawPointer(bitPattern: 0xdeadbeef)! }, | ||
let alloc = ByteBufferAllocator(hookedMalloc: { _ in UnsafeMutableRawPointer(bitPattern: 0x1337beef)! }, | ||
hookedRealloc: { _, _ in UnsafeMutableRawPointer(bitPattern: 0x1337beef)! }, | ||
hookedFree: { _ in }, | ||
hookedMemcpy: { _, _, _ in }) | ||
/* each buffer is half the writev limit */ | ||
|
@@ -708,8 +714,8 @@ public class ChannelTests: XCTestCase { | |
/// Test that with a massive buffers (bigger than writev size), we don't offer more than we should to `writev`. | ||
func testPendingWritesNoMoreThanWritevLimitIsWrittenInOneMassiveChunk() throws { | ||
let el = EmbeddedEventLoop() | ||
let alloc = ByteBufferAllocator(hookedMalloc: { _ in UnsafeMutableRawPointer(bitPattern: 0xdeadbeef)! }, | ||
hookedRealloc: { _, _ in UnsafeMutableRawPointer(bitPattern: 0xdeadbeef)! }, | ||
let alloc = ByteBufferAllocator(hookedMalloc: { _ in UnsafeMutableRawPointer(bitPattern: 0x1337beef)! }, | ||
hookedRealloc: { _, _ in UnsafeMutableRawPointer(bitPattern: 0x1337beef)! }, | ||
hookedFree: { _ in }, | ||
hookedMemcpy: { _, _, _ in }) | ||
/* each buffer is half the writev limit */ | ||
|
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.
remove the Note ?
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.
I'm also not sure why this is an
Int
.malloc
takessize_t
which is aUInt
. Maybe our allocator interface needs to change? Hope it's notpublic
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.
ping @helje5
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.
Just checked:
size_t
isunsigned int
on the 32-bit RasPi Ubuntu (I kinda assumed that it might besigned int
because quite often you can't actually alloc more than 2GB on 32-bit platforms).It probably did fail when one of the max-out tests passed in 0xDEADBEEF, which will fail the
Int(bytes)
conversion.Why you are using
Int
I don't know, I didn't introduce this. But your code says:I don't know, maybe you should just use
size_t
?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.
Moved this one to: #499.