apple / swift-atomics

Low-level atomic operations for Swift
Apache License 2.0
1.06k stars 50 forks source link

enable AtomicReference everywhere #65

Closed tayloraswift closed 1 year ago

tayloraswift commented 1 year ago

fixes #64 , and updates documentation to remove references to custom build flags.

i was not able to build the tests in their entirety, the 40k gybbed Basics.swift overwhelms my local toolchain. but i tried to make individual subsets of Basics.swift compile a little faster (~15%) by adding type annotations.

side note:

compilation time seems to grow linearly with the number of types gyb generates tests for.

n time
1 4.5s
2 10s
3 20s
4 29s
5 40s

the smaller time for the first n is probably because Int benefits from being the default inferred integer literal type.

  types = [
    ("Int",        "Int",    "12", "23"),
    ("Int8",       "Int8",   "12", "23"),
    ("Int16",      "Int16",  "12", "23"),
    ("Int32",      "Int32",  "12", "23"),
    ("Int64",      "Int64",  "12", "23"),
    ("UInt",       "UInt",   "12", "23"),
    ("UInt8",      "UInt8",  "12", "23"),
# ...
tayloraswift commented 1 year ago

update: i was able to build and run the tests and verified they all pass :)

stephentyrone commented 1 year ago

My gut feeling is that we should resolve this by unconditionally defining ENABLE_DOUBLEWIDE_ATOMICS rather than removing the checks, to make supporting other platforms that don't have support easier in the future. Otherwise someone is going to port Swift to some constrained environment and they'll have to reintroduce all the checks.

tayloraswift commented 1 year ago

i don't have a strong opinion on this, i would just prefer some version of this to be merged and tagged as soon as possible to unblock myself.

@lorentey which approach do you prefer?

tayloraswift commented 1 year ago

My gut feeling is that we should resolve this by unconditionally defining ENABLE_DOUBLEWIDE_ATOMICS rather than removing the checks, to make supporting other platforms that don't have support easier in the future. Otherwise someone is going to port Swift to some constrained environment and they'll have to reintroduce all the checks.

i might just be brainfarting here, but how do you unconditionally define ENABLE_DOUBLEWIDE_ATOMICS within a swift source module? i don’t think i have ever used the compiler directives in this way.

tayloraswift commented 1 year ago

according to https://forums.swift.org/t/how-to-use-compiler-directives-to-define-a-build-flag/63191/4 there is no way to accomplish this in swift today. @stephentyrone do you know of an alternative approach we can take here?

lorentey commented 1 year ago

Unconditionally enabling double-wide atomics is the right move. It'll need to be tied to a release that bumps the minimum toolchain version to a Swift version whose runtime also requires 16-byte atomics, but other than that this should be smooth.

lorentey commented 1 year ago

(I wouldn't like to leave the conditionals in place, unless we currently still support a platform that doesn't have wide atomics.)

tayloraswift commented 1 year ago

great! this is what the PR implements right now. anything you would like changed, or is this good to merge?

lorentey commented 1 year ago

@swift-ci test

lorentey commented 1 year ago

@swift-ci test

tayloraswift commented 1 year ago

YESS