Skip to content

Re-apply #70910 and fix 32-bit build with SWIFT_CONCURRENCY_ENABLE_PRIORITY_ESCALATION #73998

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

Merged
merged 4 commits into from
Jun 4, 2024

Conversation

nickolas-pohilets
Copy link
Contributor

Reverts #73808

Moved isDistributedRemoteActor up, as suggested by @rjmccall.

Now the layout looks like this:

0:  isa
4:  refcount
8:  isDistributedRemoteActor
12: alignment padding
16: StatusStorage
20: StatusStorage
24: StatusStorage 
28: StatusStorage 
32: PriorityQueue
36: PriorityQueue
40: PriorityQueue
44: PriorityQueue
48: PriorityQueue
52: PriorityQueue
56: trailing padding

@nickolas-pohilets nickolas-pohilets requested a review from ktoso as a code owner May 30, 2024 11:14
@nickolas-pohilets nickolas-pohilets force-pushed the mpokhylets/fix-32-build branch from f621890 to b5e039d Compare May 30, 2024 11:15
@ktoso
Copy link
Contributor

ktoso commented May 30, 2024

Thanks, I'll verify this on a 32bit box and report back!

@ktoso ktoso changed the title Re-apply https://github.com/apple/swift/pull/70910 and fix 32-bit build with SWIFT_CONCURRENCY_ENABLE_PRIORITY_ESCALATION Re-apply #70910 and fix 32-bit build with SWIFT_CONCURRENCY_ENABLE_PRIORITY_ESCALATION May 30, 2024
@ktoso
Copy link
Contributor

ktoso commented May 30, 2024

@swift-ci please test

meanwhile letting this test run here as well so it's ready to go when we are

@ktoso
Copy link
Contributor

ktoso commented May 30, 2024

@swift-ci please test

@ktoso
Copy link
Contributor

ktoso commented May 30, 2024

Either way I'll make myself a reminder to implement the "TODO: make a flagset", we should do this anyway (rdar://128999427)

@nickolas-pohilets
Copy link
Contributor Author

Fix few issues. @ktoso, could you pls start new CI job?

Implementation is quite hacky, I'm not super happy about it and open for better suggestions.

  1. I have doubts about how reliable implementation of the size_without_trailing_padding is. If it is not reliable, static_assert in Actor.cpp can give false negatives, which is pretty bad. Should we add assertions for each individual field, to be sure?
  2. Added unit-tests are probably too strict. Based on this discussion, current implementation of the unit test relies too much on Itanium ABI. For other ABI, test may fail not because there are bugs in the implementation, but because fixture types are not suitable.

@ktoso
Copy link
Contributor

ktoso commented Jun 3, 2024

Previous seemed to have passed initial testing, I'll pick the last two commits as well and run another "complete" test run, those take multiple hours, but we'll know soon.

Not sure 1) though, perhaps @rjmccall has insight on that.

In the meantime I'll verify the latest commits here, thanks for looking into this.

@ktoso ktoso requested review from rjmccall, atrick and al45tair June 3, 2024 03:11
@ktoso
Copy link
Contributor

ktoso commented Jun 3, 2024

@swift-ci please smoke test

@ktoso
Copy link
Contributor

ktoso commented Jun 3, 2024

I've confirmed 32bit issue is solved by this patch, thank you @nickolas-pohilets.

@ktoso
Copy link
Contributor

ktoso commented Jun 4, 2024

Resolves rdar://114953765

@ktoso ktoso merged commit 083fdaa into swiftlang:main Jun 4, 2024
3 checks passed
@ktoso
Copy link
Contributor

ktoso commented Jun 4, 2024

I gave this another read and as mentioned above verified internally as well.
I think we should give it another go and have prepared a pick for Swift 6.0 over here: #74112

Thanks for your effort over this long review period, thanks @nickolas-pohilets !

kastiglione added a commit that referenced this pull request Mar 25, 2025
The definition of `DefautlActorImpl` changed in #73998. This change reflects those changes on to RemoteInspection's platform independent definition.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants