Revert "Fix quadratic performance of the ListMerger in specific usage pattern"

[atrick]

Reverts #70910

This fails with a 32-bit ABIs.

In file included from swift/stdlib/public/Concurrency/Actor.cpp:26:
swift/include/swift/Basic/HeaderFooterLayout.h:27:16: error: 'padding' declared as an array with a negative size
   27 |   char padding[size];
      |                ^~~~

I suspect that the HeaderFooter TotalSize is larger than FooterSize:

      : HeaderFooterLayout<DefaultActorImplHeader, DefaultActorImplFooter,
                                sizeof(HeapObject) +
                                    sizeof(void *) * NumWords_DefaultActor> {

That's probably because DefaultActorImplHeader + DefaultActorImplFooter (PriorityQueue) is larger than
sizeof(HeapObject) + sizeof(void *) * NumWords_DefaultActor

I suggest revisiting the sizes provided to the layout, and the layout offset computation to ensure there aren't any subtle bugs. static assert may be useful in catching these problems.

[atrick]

@swift-ci smoke test

[ktoso]

FYI @nickolas-pohilets we'll have to make sure it works on 32bit before bringing it back.

[atrick]

I confirmed that reverting these commits fixes the 32-bit build.

[nickolas-pohilets]

@atrick, where can I see details of the 32-build failing? Can I reproduce it by cross-compiling on arm64 Mac?

I've tried to use command line args for compiling Actor.cpp from arm64 build and replaced arch to i386, and I could not reproduce the issue:

sizeof(void*) = 4
TotalSize = 56
sizeof(Footer) = 24
alignof(Footer) = 4
sizeof(Header) = 24
maxFooterOffset = 32
footerAlignment = 4
footerOffset = 32
size = 8

[rjmccall]

I believe the issue is with the combination of a 32-bit target and SWIFT_CONCURRENCY_ENABLE_PRIORITY_ESCALATION, since that makes sizeof(ActiveActorStatus) 16 instead of 8. StatusStorage is declared with 16-byte alignment in this configuration, so sizeof(Header) is probably 48:

0:  isa
4:  refcount
8:  alignment padding
12: alignment padding
16: StatusStorage
20: StatusStorage
24: StatusStorage 
28: StatusStorage 
32: isDistributedRemoteActor
36: tail padding
40: tail padding
44: tail padding

That is, the header is already using 10 of the 12 words available in NumWords_DefaultActor, 5 of which are totally empty. The C++ ABI has an optimization to lay subsequent members in the tail padding of non-POD bases, but I believe Header is POD for those purposes, and it wouldn't be good enough anyway since IIRC we need 6 words in the footer and we'd only get 5.

I think there's a relatively simple fix: you need to move isDistributedRemoteActor prior to StatusStorage in this configuration, which will knock 4 words off Header. (Alternatively, you could merge isDistributedRemoteActor into the flags stored in the status storage, which there's a longstanding TODO for.) HeaderFooterLayout will need to make sure it doesn't insert any padding when the footer and header combine for the full size.

[nickolas-pohilets]

I can make the changes, but my main struggle is to reproduce the issue and test the changes. Looks like I need <dispatch/swift_concurrency_private.h> to build this case, which does not seem to be publicly available.

[rjmccall]

I can make the changes, but my main struggle is to reproduce the issue and test the changes. Looks like I need <dispatch/swift_concurrency_private.h> to build this case, which does not seem to be publicly available.

dispatch_lock_t is ultimately just a uint32_t, if you want to hack things up to get the right layout.

[nickolas-pohilets]

I think there's a relatively simple fix: you need to move isDistributedRemoteActor prior to StatusStorage in this configuration, which will knock 4 words off Header. (Alternatively, you could merge isDistributedRemoteActor into the flags stored in the status storage, which there's a longstanding TODO for.) HeaderFooterLayout will need to make sure it doesn't insert any padding when the footer and header combine for the full size.

After moving isDistributedRemoteActor and updating HeaderFooterLayout to handle zero padding, I'm still getting sizeof(DefaultActorImpl) == 64, where 56 bytes are actually used, and the last 8 is just a trailing padding, because entire DefaultActorImpl gets its alignment of 16 from ActiveActorStatus.

I can loosen static_assert from sizeof(DefaultActorImpl) <= TotalSize to something like this:

class DefaultActorImpl: public HeaderFooterLayout<DefaultActorImplHeader, DefaultActorImplFooter, TotalSize> {
  char bytesPastTheEnd[1];
  ...
};

static_assert(offsetof(DefaultActorImpl, bytesPastTheEnd) <= TotalSize);

[nickolas-pohilets]

Alternatively, you could merge isDistributedRemoteActor into the flags stored in the status storage, which there's a longstanding TODO for.

I don't think it this TODO is about that. IMO, it is about replacing bool with a bit inside a FlagSet<>. Variable isDistributedRemoteActor is immutable, I don't think it makes sense to store it inside ActiveActorStatus which is accessed atomically.