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

Fast atomics #1263

Merged
merged 16 commits into from Nov 28, 2019
Merged

Fast atomics #1263

merged 16 commits into from Nov 28, 2019

Conversation

@2bjake
Copy link
Contributor

2bjake commented Nov 21, 2019

Motivation:

The existing Atomic class holds an UnsafeEmbeddedAtomic which holds an OpaquePointer to raw memory for the C atomic. This results in multiple allocations on init. The new FastAtomic class uses ManagedBufferPointer which tail allocates memory for the C atomic as part of the FastAtomic class allocation.

Modifications:

Created FastAtomic class that uses ManagedBufferPointer to allocate memory for the C atomic. Created version of catmc_atomic_* C functions that expect memory to be allocated/deallocated by caller. Created FastAtomicPrimitive protocol for the new version of catmc_atomic_* functions.

Result:

Purely additive. FastAtomic is available as an alternative to Atomic which results in fewer memory allocations.

2bjake added 2 commits Nov 21, 2019
Motivation:

The existing Atomic class holds an UnsafeEmbeddedAtomic which holds an OpaquePointer to raw memory for the C atomic. This results in multiple allocations on init. The new FastAtomic class uses ManagedBufferPointer which tail allocates memory for the C atomic as part of the FastAtomic class allocation.

Modifications:

Created FastAtomic class that uses ManagedBufferPointer to allocate memory for the C atomic. Created version of catmc_atomic_* C functions that expect memory to be allocated/deallocated by caller. Created FastAtomicPrimitive protocol for the new version of catmc_atomic_* functions.

Result:

Purely additive. FastAtomic is available as an alternative to Atomic which results in fewer memory allocations.
@swift-server-bot

This comment has been minimized.

Copy link

swift-server-bot commented Nov 21, 2019

Can one of the admins verify this patch?

8 similar comments
@swift-server-bot

This comment has been minimized.

Copy link

swift-server-bot commented Nov 21, 2019

Can one of the admins verify this patch?

@swift-server-bot

This comment has been minimized.

Copy link

swift-server-bot commented Nov 21, 2019

Can one of the admins verify this patch?

@swift-server-bot

This comment has been minimized.

Copy link

swift-server-bot commented Nov 21, 2019

Can one of the admins verify this patch?

@swift-server-bot

This comment has been minimized.

Copy link

swift-server-bot commented Nov 21, 2019

Can one of the admins verify this patch?

@swift-server-bot

This comment has been minimized.

Copy link

swift-server-bot commented Nov 21, 2019

Can one of the admins verify this patch?

@swift-server-bot

This comment has been minimized.

Copy link

swift-server-bot commented Nov 21, 2019

Can one of the admins verify this patch?

@swift-server-bot

This comment has been minimized.

Copy link

swift-server-bot commented Nov 21, 2019

Can one of the admins verify this patch?

@swift-server-bot

This comment has been minimized.

Copy link

swift-server-bot commented Nov 21, 2019

Can one of the admins verify this patch?

@Lukasa Lukasa requested a review from weissi Nov 22, 2019
@Lukasa Lukasa added this to the 2.11.0 milestone Nov 22, 2019
Copy link
Contributor

Lukasa left a comment

This generally looks really good, well done! I have just a few notes inline.

Sources/CNIOAtomics/include/CNIOFastAtomics.h Outdated Show resolved Hide resolved
Sources/CNIOAtomics/src/c-fastatomics.c Outdated Show resolved Hide resolved
Sources/CNIOAtomics/include/CNIOFastAtomics.h Outdated Show resolved Hide resolved
Sources/NIOConcurrencyHelpers/fastatomics.swift Outdated Show resolved Hide resolved
Sources/NIOConcurrencyHelpers/fastatomics.swift Outdated Show resolved Hide resolved
Sources/NIOConcurrencyHelpers/fastatomics.swift Outdated Show resolved Hide resolved
@Lukasa
Lukasa approved these changes Nov 22, 2019
Copy link
Contributor

Lukasa left a comment

Cool, this LGTM. @weissi?

/// Create an atomic object with `value`
@inlinable
static func makeAtomic(value: T) -> FastAtomic {
let manager = Manager(bufferClass: self, minimumCapacity: 1) { _, _ in }

This comment has been minimized.

Copy link
@weissi

weissi Nov 22, 2019

Member

@atrick the only reason we can use an UInt8 buffer of the right size is that we need memory that’s sufficiently aligned for an atomic. I assume there are no alignment guarantees for this?

This comment has been minimized.

Copy link
@atrick

atrick Nov 22, 2019

Member

@weissi I'm not sure what you're asking. ManagedBuffer gives you the alignment guarantee per its _alignmentMask. Unsafe[Buffer]Pointer<T>.allocate would give you the same guarantee. Or you can explicitly provide alignment to UnsafeRawPointer.allocate

This comment has been minimized.

Copy link
@Lukasa

Lukasa Nov 25, 2019

Contributor

I'm a bit confused by @weissi's question here: we're not allocating a UInt8 buffer, we're allocating a buffer of type T.AtomicWrapper of size 1. This does not seem like it would have any alignment issues at all.

This comment has been minimized.

Copy link
@weissi

weissi Nov 25, 2019

Member

@atrick / @Lukasa sorry, I was on a phone only on Friday. What we really want to express here is tail allocation for an opaque (forward declared) struct in C. Of course because it's opaque, we don't know it's true length/alignment requirements. We could however expose getters for length & alignment.

That way, we could make a ManagedBufferPointer that holds length UInt8s. Then we could pass the UnsafeMutableRawBufferPointer to these UInt8s to the C function that initialises the atomic. The only issue here is that our UnsafeMutableRawBufferPointer isn't correctly aligned to fit the struct that contains the atomic.

And I was wondering if we could ask ManagedBufferPointer to give us tail allocated storage aligned to a user-chosen alignment. So something like

let manager = Manager<Void, UInt8>(bufferClass: self, minimumCapacity: get_width_for_opaque_struct(), alignment: get_alignment_requirement_for_opaque_struct()) { _, _, in }

But this API doesn't seem to exist. We could obviously ask for a possibly larger size and align ourselves.

(and we'd ask for a number of UInt8s and then initialise them in C to a certain struct which isn't valid if we would store a typed UnsafeBufferPointer<UInt8> in Swift because we'd pun that pointer to opaque_struct in C).

This comment has been minimized.

Copy link
@atrick

atrick Nov 25, 2019

Member

@weissi Ah. I get it. I think it would be reasonable to propose adding an (over)alignment option to ManagedBuffer.create. I'm not sure if we can add an underscored, defaulted argument to a public API. Might need to add a new underscored API instead. Without an API I think you need to allocate extra space.

#include "cpp_magic.h"

#define MAKE(type) /*
*/ void catmc_fast_atomic_##type##_create_with_existing_storage(struct catmc_fast_atomic_##type *storage, type value) { /*

This comment has been minimized.

Copy link
@weissi

weissi Nov 22, 2019

Member

@2bjake instead of duplicating everything here, couldn’t we just declare the existing structs in the header like you do for the fast atomics? There’s no actual need to only forward declare them. I’m sure we can get away with just one copy of the C stuff

This comment has been minimized.

Copy link
@2bjake

2bjake Nov 22, 2019

Author Contributor

I originally tried this but declaring the structs in the header causes the generated Swift interface of these functions to change. This means the AtomicPrimitive protocol would have to change as well (compare AtomicPrimitive and FastAtomicPrimitive). Wouldn't this result in a backward incompatible API change?

This comment has been minimized.

Copy link
@2bjake

2bjake Nov 22, 2019

Author Contributor

Wait, I see a way where the pointer type of AtomicPrimitive methods wouldn't have to change by moving away from pointfree conformance and wrapping the c functions. Something like:

public static let opaque_add: (OpaquePointer, Self) -> Self = { catmc_atomic__Bool_add(UnsafeMutablePointer<AtomicWrapper>($0), $1) }

But there will still need to be a FastAtomicPrimitive protocol, right? So I can add the create_with_storage method and the AtomicWrapper associatedtype in a non-breaking way.

This comment has been minimized.

Copy link
@2bjake

2bjake Nov 22, 2019

Author Contributor

OK, I was able to remove the duplicate c code. I was also able to dramatically reduce the boilerplate for AtomicPrimitive conformance by providing default implementation computed properties which rely on FastAtomicPrimitive!

Copy link
Member

weissi left a comment

This is amazing, thank you! I think we’re almost there, just two things I think we can fix:

  • I don’t think we need to duplicate the C code
  • I think we should make the existing Atomics be inplemented with FastAtomic (just UnsafeEmbeddedAtomic cannot use FastAtomic)
@2bjake

This comment has been minimized.

Copy link
Contributor Author

2bjake commented Nov 22, 2019

@weissi I'm not sure how I can make Atomic be implemented by FastAtomic since they're generic over different types (AtomicPrimitive vs FastAtomicPrimitive). I was able to convert AtomicBox over to FastAtomic though.

return Manager(unsafeBufferObject: self).withUnsafeMutablePointerToElements {
return T.fast_atomic_exchange($0, value)
Comment on lines 230 to 231

This comment has been minimized.

Copy link
@2bjake

2bjake Nov 23, 2019

Author Contributor

@weissi @Lukasa What's your stance on implicit returns? Both returns in each of these functions can be elided but I included them since I don't know the preferred style.

This comment has been minimized.

Copy link
@Lukasa

Lukasa Nov 25, 2019

Contributor

Mixed. We tend to allow them within single-statement closures, but as we support Swift 5 we forbid them everywhere else.

@weissi

This comment has been minimized.

Copy link
Member

weissi commented Nov 25, 2019

@swift-nio-bot test this please

@weissi

This comment has been minimized.

Copy link
Member

weissi commented Nov 25, 2019

@2bjake unfortunately, the API breakage checker found breakages here: https://ci.swiftserver.group/job/swift-nio2-api-breakage-prb/406/console

Essentially, we need to keep the OpaquePointer APIs depite the fact we now accept UnsafePointer<concrete_type>

@2bjake

This comment has been minimized.

Copy link
Contributor Author

2bjake commented Nov 25, 2019

@weissi The types of the original AtomicPrimitive APIs are unchanged and still deal in OpaquePointers. Is it an issue that they are now provided as default implementations on a protocol extension?

@weissi

This comment has been minimized.

Copy link
Member

weissi commented Nov 25, 2019

@2bjake

[...]
 17:12:39 /* Type Changes */
17:12:39 Accessor Bool.atomic_add.Get() has return type change from (OpaquePointer, Bool) -> Bool to (UnsafeMutablePointer<catmc_atomic__Bool>, Bool) -> Bool
17:12:39 Accessor Bool.atomic_compare_and_exchange.Get() has return type change from (OpaquePointer, Bool, Bool) -> Bool to (UnsafeMutablePointer<catmc_atomic__Bool>, Bool, Bool) -> Bool
17:12:39 Accessor Bool.atomic_exchange.Get() has return type change from (OpaquePointer, Bool) -> Bool to (UnsafeMutablePointer<catmc_atomic__Bool>, Bool) -> Bool
17:12:39 Accessor Bool.atomic_load.Get() has return type change from (OpaquePointer) -> Bool to (UnsafeMutablePointer<catmc_atomic__Bool>) -> Bool
17:12:39 Accessor Bool.atomic_store.Get() has return type change from (OpaquePointer, Bool) -> Void to (UnsafeMutablePointer<catmc_atomic__Bool>, Bool) -> Void
17:12:39 Accessor Bool.atomic_sub.Get() has return type change from (OpaquePointer, Bool) -> Bool to (UnsafeMutablePointer<catmc_atomic__Bool>, Bool) -> Bool
17:12:39 Accessor Int.atomic_add.Get() has return type change from (OpaquePointer, Int) -> Int to (UnsafeMutablePointer<catmc_atomic_long>, Int) -> Int
17:12:39 Accessor Int.atomic_compare_and_exchange.Get() has return type change from (OpaquePointer, Int, Int) -> Bool to (UnsafeMutablePointer<catmc_atomic_long>, Int, Int) -> Bool
17:12:39 Accessor Int.atomic_exchange.Get() has return type change from (OpaquePointer, Int) -> Int to (UnsafeMutablePointer<catmc_atomic_long>, Int) -> Int
17:12:39 Accessor Int.atomic_load.Get() has return type change from (OpaquePointer) -> Int to (UnsafeMutablePointer<catmc_atomic_long>) -> Int
17:12:39 Accessor Int.atomic_store.Get() has return type change from (OpaquePointer, Int) -> Void to (UnsafeMutablePointer<catmc_atomic_long>, Int) -> Void
17:12:39 Accessor Int.atomic_sub.Get() has return type change from (OpaquePointer, Int) -> Int to (UnsafeMutablePointer<catmc_atomic_long>, Int) -> Int
17:12:39 Accessor Int16.atomic_add.Get() has return type change from (OpaquePointer, int_least16_t) -> int_least16_t to (UnsafeMutablePointer<catmc_atomic_int_least16_t>, int_least16_t) -> int_least16_t
@weissi

This comment has been minimized.

Copy link
Member

weissi commented Nov 25, 2019

@2bjake the problem is that a some_struct * in imported into Swift in two different ways:

  1. UnsafePointer<some_struct> if struct some_struct is fully declared in the header file
  2. OpaquePointer if struct some_struct is forward declared

Previously, we had situation (2) but now it's (1). That's not a problem per se because we should be able to maintain the OpaquePointer version, no?

@2bjake

This comment has been minimized.

Copy link
Contributor Author

2bjake commented Nov 26, 2019

Ok, with the deduplication commit reverted, allocations go from 554000 down to 548000 because AtomicBox is using FastAtomic. I then converted the usages of Atomic throughout the codebase to FastAtomic and allocations dropped further to 543000.

* Add documentation to FastAtomic
* Make FastAtomic final and makeAtomic() public
* Add deprecation warning to Atomic
* Change all uses of Atomic (except concurrency src & test) to use FastAtomic
@2bjake

This comment has been minimized.

Copy link
Contributor Author

2bjake commented Nov 26, 2019

@weissi My username on that JIRA is jakef.

@weissi

This comment has been minimized.

Copy link
Member

weissi commented Nov 26, 2019

@swift-nio-bot test this please

@2bjake

This comment has been minimized.

Copy link
Contributor Author

2bjake commented Nov 27, 2019

TheAtomic unit tests are getting deprecation warnings which causes validation to fail. Any thoughts on how to handle this?

@weissi

This comment has been minimized.

Copy link
Member

weissi commented Nov 27, 2019

@2bjake there seem to be warnings in the code now, mind just switching those to FastAtomic<T>?

21:24:38 [13/85] Compiling NIO BaseSocket.swift
21:24:38 /code/Tests/NIOConcurrencyHelpersTests/NIOConcurrencyHelpersTests.swift:34:40: error: 'Atomic' is deprecated: please use FastAtomic instead
21:24:38         let ai = NIOConcurrencyHelpers.Atomic<UInt64>(value: 0)
21:24:38                                        ^
21:24:38 /code/Tests/NIOConcurrencyHelpersTests/NIOConcurrencyHelpersTests.swift:47:18: error: 'Atomic' is deprecated: please use FastAtomic instead
21:24:38         let ab = Atomic<Bool>(value: true)
21:24:38                  ^
21:24:38 /code/Tests/NIOConcurrencyHelpersTests/NIOConcurrencyHelpersTests.swift:60:18: error: 'Atomic' is deprecated: please use FastAtomic instead
21:24:38         let ab = Atomic<Bool>(value: false)
21:24:38                  ^
21:24:38 /code/Tests/NIOConcurrencyHelpersTests/NIOConcurrencyHelpersTests.swift:80:22: error: 'Atomic' is deprecated: please use FastAtomic instead
21:24:38             let ab = Atomic<T>(value: max)
21:24:38                      ^
21:24:38 /code/Tests/NIOConcurrencyHelpersTests/NIOConcurrencyHelpersTests.swift:110:22: error: 'Atomic' is deprecated: please use FastAtomic instead
21:24:38             let ab = Atomic<T>(value: upperBound)
21:24:38                      ^
21:24:38 /code/Tests/NIOConcurrencyHelpersTests/NIOConcurrencyHelpersTests.swift:140:22: error: 'Atomic' is deprecated: please use FastAtomic instead
21:24:38             let ab = Atomic<T>(value: zero)
21:24:38                      ^
21:24:38 /code/Tests/NIOConcurrencyHelpersTests/NIOConcurrencyHelpersTests.swift:171:22: error: 'Atomic' is deprecated: please use FastAtomic instead
21:24:38             let ab = Atomic<T>(value: zero)
21:24:38                      ^
21:24:38 /code/Tests/NIOConcurrencyHelpersTests/NIOConcurrencyHelpersTests.swift:202:22: error: 'Atomic' is deprecated: please use FastAtomic instead
21:24:38             let ab = Atomic<T>(value: zero)
21:24:38                      ^
21:24:39 Build step 'Execute shell' marked build as failure
@2bjake

This comment has been minimized.

Copy link
Contributor Author

2bjake commented Nov 27, 2019

@weissi those are the unit tests for Atomic. Switching that code to FastAtomic makes them worthless (we already have FastAtomic unit tests). Should I remove them? Or do you have a way to have tests against deprecated code without causing validation errors?

@weissi

This comment has been minimized.

Copy link
Member

weissi commented Nov 27, 2019

@2bjake if you rebase your branch on top of master, you can use this trick: https://github.com/apple/swift-nio/blob/master/Tests/NIOTests/ByteBufferTest.swift#L2445

Just deprecate the tests that use deprecated functions :)

ktoso and others added 5 commits Nov 26, 2019
Fix typo in Endianness docs

Motivation:

Slight typo in docs of Endianness

Modifications:

`significat` -> `significant`

Result:

Less typos, happier users.
Motivation:

ByteBufferView isn't a mutable collection, but it probably should be.

Modifications:

- Add `copyBytes(at:to:length)` to `ByteBuffer` to copy bytes from a
  readable region of a buffer to another part of the buffer
- Conform `ByteBufferView` to `MutableCollection`, `RangeReplaceableCollection`
  and `MutableDataProtocol`
- Add an allocation counting test

Result:

`ByteBufferView` is now mutable.
Motivation:

It's important to also test deprecated functionliaty. One way of
achieving this without warnings is to also deprecate the tests that test
this deprecated functionality. Unfortunately, on Linux we need to
generate lists of tests which would then reference deprecated tests
(which gives us a warning).

Modifications:

Deprecate test suites and the main test runner all the way to the top so
never get warnings.

Result:

Possible to test deprecated functionlity without warnings.
@weissi

This comment has been minimized.

Copy link
Member

weissi commented Nov 27, 2019

@swift-nio-bot test this please

@2bjake

This comment has been minimized.

Copy link
Contributor Author

2bjake commented Nov 27, 2019

@weissi I think the 5.1 validation error is a false alarm, it's failing because total_allocations is lower than expected, which is a good thing (this is what I saw on my machine). I'm not sure what's going on with the 5.0 validation error, allocs went up by 5, from 30970 to 30975.

@weissi

This comment has been minimized.

Copy link
Member

weissi commented Nov 27, 2019

@2bjake awesome work! To make this actually pass CI, you can now do what is for me the favourite part of a PR that optimises allocations: You can edit the two docker files in your PR and subtract the right number of allocations your PR saves :).

So if you save 4 allocations, you typically subtract 4000 from the limits (because we usually run stuff 1000 times). Usually, there's some extra slack at the end to account for random stray allocations. So for example if we have a benchmark that allocates 12 times per round and we run it 1000 times, then I'd usually set a limit of 12050 == (12 times * 1000 allocations) + 50 slack.

Does that make sense?

@weissi

This comment has been minimized.

Copy link
Member

weissi commented Nov 27, 2019

@2bjake for the one where it went up a tiny little bit (from 30790 to 30795, feel free to increase the slack to 30799 or something). Just needs to be below 31000.

@weissi

This comment has been minimized.

Copy link
Member

weissi commented Nov 27, 2019

@swift-nio-bot add to whitelist

@2bjake

This comment has been minimized.

Copy link
Contributor Author

2bjake commented Nov 27, 2019

@weissi Allocations in the conn tests went down in 5.1 but went up in 5.0.

/// 5.1 ///
MAX_ALLOCS_ALLOWED_1000_reqs_1_conn=30550
test_1000_reqs_1_conn.total_allocations: 30492

MAX_ALLOCS_ALLOWED_1_reqs_1000_conn=533050
test_1_reqs_1000_conn.total_allocations: 522000

MAX_ALLOCS_ALLOWED_ping_pong_1000_reqs_1_conn=4450
test_ping_pong_1000_reqs_1_conn.total_allocations: 4434

/// 5.0 ///
MAX_ALLOCS_ALLOWED_1000_reqs_1_conn=30970
test_1000_reqs_1_conn.total_allocations: 30975

MAX_ALLOCS_ALLOWED_1_reqs_1000_conn=994050
test_1_reqs_1000_conn.total_allocations: 1005000

MAX_ALLOCS_ALLOWED_ping_pong_1000_reqs_1_conn=4480
test_ping_pong_1000_reqs_1_conn.total_allocations: 4489

I poked around online but didn't find anything about the 5.1 release that would explain it. A total guess, but I'm wondering if the capturing closures that are passed to withUnsafeMutablePointerToElements cause allocations in 5.0 but are optimized in 5.1? The FastAtomic implementation has a bunch of them.

@weissi

This comment has been minimized.

Copy link
Member

weissi commented Nov 27, 2019

@2bjake that's great! We still care about 5.0 working but performance-wise 5.1 is definitely the target. So I think we're more than happy with 5.1 becoming better for 5.0 becoming worse :).

Just adjust the docker files accordingly and then this is good to go :).

@weissi

This comment has been minimized.

Copy link
Member

weissi commented Nov 27, 2019

@2bjake oh damn, one annoying thing: according to our public API declaration (specifically this paragraph) we have to prefix all new global type/protocol names with NIO. So we'd need to rename them to NIOFastAtomicPrimitive and NIOFastAtomic. That makes me think: let's just use NIOAtomic maybe?

The reason for the NIO prefixes is to "guarantee" that we won't clash with other people's types which would break SemVer in Swift :(

2bjake and others added 4 commits Nov 27, 2019
@weissi
weissi approved these changes Nov 27, 2019
Copy link
Member

weissi left a comment

Amazing work! This is good to go from my point of view

@Lukasa
Lukasa approved these changes Nov 28, 2019
Copy link
Contributor

Lukasa left a comment

Very nice, great work!

@Lukasa Lukasa merged commit 3c879eb into apple:master Nov 28, 2019
4 checks passed
4 checks passed
pull request validation (5.0) Build finished.
Details
pull request validation (5.1) Build finished.
Details
pull request validation (api breakage) Build finished.
Details
pull request validation (sanity) Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.