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.

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

Add support for compiling and running NIO on Raspberry Pi 32-bit #383

Merged
merged 8 commits into from
May 4, 2018
Merged

Add support for compiling and running NIO on Raspberry Pi 32-bit #383

merged 8 commits into from
May 4, 2018

Conversation

helje5
Copy link
Contributor

@helje5 helje5 commented May 2, 2018

Port SwiftNIO to 32-bit platforms

Motivation:

Pretty much every developer in the world want to run MicroExpress applications on Raspberry Pi K8s clusters. The unfortunate precondition to that is getting NIO compiled for RaspberryPi. This PR makes it work.

Thanks go to @chnmrc for providing a Swift 4.1 Raspi build, also available via a DockerHub: helje5/rpi-swift.

Modifications:

Two main changes:

  1. Timestamps used to be stored as Int, this needs to be an explicit Int64 to provide the necessary value range for ns values
  2. Some hardcoded pointers like 0xdeafbeef got mapped to Int values, but Int32 can't represent 0xdeadbeef. Needs to be UInt32 to capture the full deadness.

Result:

NIO compiles on Raspi 3, 32-bit Ubuntu. I tried the Echo Server, which seems to work, and also the HTTP sample Server, which also seems to work.

P.S.: This should also allow you to run NIO on AppleWatch, which is 32-bit too.

helje5 added 5 commits May 2, 2018 13:26
Motivation:

Pointers are unsigned on the supported platforms.
This makes NIO compile on 32 bit platforms, like
AppleWatch.

The compilation error is actually not because of
UInt, but because of 0xDEADBEEF, which requires
full 32 bit width.

Modifications:

Changed `AtomicBox` storage from `Int` to `UInt`.

Result:

NIO compiles on 32 bit platforms.
Motivation:

When building a datacenter out of Apple Watches w/ shattered
glasses, you'll need the ability to build on 32 bit platforms.
This fixes building NIOHTTP1 on such platforms.

Modifications:

This was resetting the parser context pointer using a very
long deadly beef. Had to shorted this for 32bit cows.

Result:

NIOHTTP1 compiles on 32 bit ARM platforms.
Motivation:

On 32-bit platforms the ByteBuffer is too big indeed. This is
not a real fix for that, but at least it makes it work on
32-bit ARM, even if it will be very slow ...

Modifications:

Disabled assert on 32-bit ARM.

Result:

NIO programs do not crash right away on Apple Watch compute
clustes.
Motivation:

To run k8s NIO clusters on Apple Watches, we need to
make the software 32-bit safe.

NIO uses a lot of timestamps directly (w/o a dedicated
type). And it uses `Int` for those, which is insufficient
for the desired ns granularity on 32-bit platforms.

I think the proper fix would be to use a proper custom
time type, but until then, lets use `Int64` when time
values are involved.

Modifications:

Replace Int w/ Int64 for time calculations.

Result:

Echo daemon runs on Apple Watch. If you just shout Hello
at it, it doesn't answer yet though.
Motivation:

Make sourcecode look consistent. Die \t die.

Modifications:

Fix indent

Result:

Sources look consistent regardless of tab width.
@swift-nio-bot
Copy link

Can one of the admins verify this patch?

1 similar comment
@swift-nio-bot
Copy link

Can one of the admins verify this patch?

@Lukasa Lukasa added the ⚠️ semver/major Breaks existing public API. label May 2, 2018
parser.data = UnsafeMutableRawPointer(bitPattern: veryDeadBeef)
#else
parser.data = UnsafeMutableRawPointer(bitPattern: 0x0000deadbeef0000)
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

@jw Do we need this change? I think we only wanted this pointer value to be easy to spot in debuggers and core dumps: does it need to have the deadbeef portion shifted left 4 bits on 64-bit operating systems or is it ok to leave it in the least significant 32 bits?

Copy link
Member

Choose a reason for hiding this comment

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

we can just use a 32 bit value 0xdeadbeef is totally fine by me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That still needs the UInt32 cast though, because parser.data seems to be Int32.

Copy link
Member

Choose a reason for hiding this comment

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

@helje5 isn’t parser.data a pointer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right it is. I guess the problem is the Swift type overloading again. It will treat 0xdeadbeef as an Int. At least that's the error w/o the explicit UInt (cannot represent Integer literal as Int or something like that).

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, but can't we just compress this into one line? parser.data = UnsafeMutableRawPointer(bitPattern: UInt(0xdeadbeef)? That'll work correctly on all platforms.

Copy link
Member

Choose a reason for hiding this comment

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

@helje5 I'm happy with this except for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should, but you have to remember that we are talking Swift here! ;->

/src/Sources/NIOHTTP1/HTTPDecoder.swift:382:64: error: integer literal '3735928559' overflows when stored into 'Int'
        parser.data = UnsafeMutableRawPointer(bitPattern: UInt(0xdeadbeef))

Same issue: UInt essentially has a UInt.init(Int), hence Swift will treat the literal as such ...

@Lukasa
Copy link
Contributor

Lukasa commented May 2, 2018

I have...mixed feelings about this.

For NIO's major server-side use-cases 32-bit architectures are not compelling to target. While the convenience of running on a Raspberry Pi is nice, it's certainly not a feature I feel compelled to support.

Supporting it would be easier if it we had CI for 32-bit environments, but currently we don't, so the risk of regression here is high. I think we probably need to decide as a team whether we care about supporting 32-bit environments at all: if we do, then we can merge this for 2.0, and if we don't, we can skip it.

@helje5
Copy link
Contributor Author

helje5 commented May 2, 2018

I think you should treat this similar to Swift itself - no official support at all, but accept community PRs which make it work.

@helje5
Copy link
Contributor Author

helje5 commented May 2, 2018

BTW: Why do you think this needs a major version bump?

@ianpartridge
Copy link
Contributor

I think supporting Raspberry Pi is worthwhile. Swift is attractive to embedded/IoT devices, and there are many things which could be built on top of NIO that would be valuable on Raspberry Pi and similar devices.

For example, https://mosquitto.org/ is a widely deployed MQTT broker that runs on Raspberry Pi and it would be really nice if there was a NIO-based MQTT broker.

assert(MemoryLayout<ByteBuffer>.size <= 3 * MemoryLayout<Int>.size,
"ByteBuffer has size \(MemoryLayout<ByteBuffer>.size) which is larger than the built-in storage of the existential containers.")
#if !arch(arm) // only complain on 64-bit, this is unfortunate reality on 32-bit
assert(MemoryLayout<ByteBuffer>.size <= 3 * MemoryLayout<Int>.size,
Copy link
Member

Choose a reason for hiding this comment

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

I think we can just kill this assertion. We now have much better tests (allocation counters) which will blow up if this assertion would fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume there is no chance to make this work better on 32-bit?

Copy link
Member

Choose a reason for hiding this comment

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

@helje5 the existential buffer size is 3 words. On 32 bit that means 12 bytes. Given that byte buffer needs one pointer, we’re left with 8 bytes for reader index, writer index, slice begin and end. I don’t think it’s feasible. So unfortunately for 32bit we’ll need to be larger than the builtin storage of an existential. But we still have the byte buffer special case in NIOAny so not all too bad...

@Lukasa
Copy link
Contributor

Lukasa commented May 2, 2018

BTW: Why do you think this needs a major version bump?

@helje5 The changes on TimeAmount are all semver-major.

@weissi
Copy link
Member

weissi commented May 2, 2018

@helje5 / @Lukasa we could just make a hack and only make it Int64 for 32 bit platforms. I know that's not how it's meant to be used but I'd be okay with that.

@Lukasa
Copy link
Contributor

Lukasa commented May 2, 2018

@weissi Any reason not to just make it an Int64 but provide deprecated compat shims to the old type, so that it can still be accessed and set via Int?

@helje5
Copy link
Contributor Author

helje5 commented May 2, 2018

TBH this kind of code doesn't feel quite right in the first place when you have your own TimeAmount abstraction:

Int(DispatchTime.now().uptimeNanoseconds) + amount.nanoseconds

This code probably belongs into a new Timestamp type, and/or the TimeAmount.

@chnmrc
Copy link

chnmrc commented May 3, 2018

Mqtt broker on top of this
https://www.netiot.com/netpi/industrial-raspberry-pi-3/

@futurejones
Copy link

Supporting it would be easier if it we had CI for 32-bit environments, but currently we don't, so the risk of regression here is high. I think we probably need to decide as a team whether we care about supporting 32-bit environments at all.

As Swift is supporting 32-bit environments I think it is important that swift-nio also supports 32-bit.

We now have CI for Armv7 32-bit environments.
https://swift.org/blog/swift-community-hosted-ci/

Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

I think after changing this PR not to change the public API on 64bit platforms we should take this. Ping @normanmaurer

@weissi
Copy link
Member

weissi commented May 4, 2018

@swift-nio-bot test this please

@normanmaurer
Copy link
Member

@weissi Agree as long as public API stays the same on 64bit I am happy with the PR

@helje5
Copy link
Contributor Author

helje5 commented May 4, 2018

I will do the #if fix for the 1.x line. Neat shims are a little harder due to the nanoseconds property. In a different PR we should pull out the time calculations out of the main code base into TimeAmount, but let’s just go with the #if arm for this PR

Motivation:

`Int64` is the right choice here, but it breaks SemVer
and needs to wait for NIO 2.x. So instead we just use
`Int64` on ARM 32-bit, and stick w/ `Int` on the other
platforms.

Modifications:

Added a typealias `TimeAmount.Value` which aliases to
`Int` on 64-bit platforms and `Int64` on arch(arm).

Result:

Result should be SemVer minor.
@helje5
Copy link
Contributor Author

helje5 commented May 4, 2018

@weissi @Lukasa PTAL, I added a typealias which should make it SemVer minor. OK with that?

BTW: Small hint: You don't need an actual Raspi to test a Raspi build! If you have Docker4Mac, it has QEmu included and can run simple Raspi images (it can't run NIO servers, unfortunately). To build NIO from within a NIO checkout:

docker run --rm \
		-v $(pwd):/src \
		-v $(pwd)/.build-linux-rpi-4.1.0:/src/.build \
		helje5/rpi-swift-dev:4.1.0 \
		bash -c "cd /src && swift build -c release"

$ file .build-linux-rpi-4.1.0/release/NIOWebSocketServer
.build-linux-rpi-4.1.0/release/NIOWebSocketServer: ELF 32-bit LSB shared object, ARM, EABI5 version 1 (SYSV), dynamically linked, interpreter /lib/ld-linux-armhf.so.3, for GNU/Linux 3.2.0, BuildID[sha1]=f9efd4dbdc64cea811c296315ca83daa8233b00e, not stripped

Takes about 2.5 mins on my MacPro.

helje5 and others added 2 commits May 4, 2018 17:37
Motivation:

Make code nicer.

Modifications:

Use 0xdeadbeef on both, 64-bit and 32-bit. Cast using `as`.

Result:

The source code will be pure beauty. (not, 4-space indent!!)
@normanmaurer
Copy link
Member

@swift-nio-bot test this please

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@Lukasa Lukasa added 🔨 semver/patch No public API change. and removed ⚠️ semver/major Breaks existing public API. labels May 4, 2018
@Lukasa Lukasa added this to the 1.7.0 milestone May 4, 2018
Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

great work, thanks @helje5

@Lukasa Lukasa merged commit 7fcbff9 into apple:master May 4, 2018
@helje5 helje5 mentioned this pull request Jun 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants