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

[6.0.1] Add new Foundation libraries to SwiftRuntimeLibsOrdered and update others (#76019) #76606

Closed
wants to merge 2 commits into from

Conversation

finagolfin
Copy link
Member

Explanation: Changes a list of automatically de-duped libraries passed to the linker to reduce memory allocated while linking new Foundation and runtime libraries

Scope: Updates a list of libraries that already contains Foundation to add additional Foundation and runtime libraries

Issue: swiftlang/swift-foundation#872

Original PRs: #76019 and #76224

Risk: Low - scope is narrow and tested with the already existing Foundation and runtime libraries in the list

Testing: Testing done via swift-ci testing

Reviewer: @Azoy @etcwilde @jmschonfeld

There was some concern from others about removing the libicu de-duplication in #76224 for this 6.0 branch, but I'm fairly certain that is entirely unused after the Foundation re-core to use _FoundationICU instead, as I noted there.

@shahmishal, this may fix #76555.

@finagolfin finagolfin requested a review from a team as a code owner September 20, 2024 16:23
@finagolfin finagolfin changed the title [6.0.1] Add new Foundation libraries to SwiftRuntimeLibsOrdered (#76019) [6.0.1] Add new Foundation libraries to SwiftRuntimeLibsOrdered and update others (#76019) Sep 20, 2024
@shahmishal
Copy link
Member

I would like to hear @parkera thoughts on pulling this in to 6.0.1 release.

@shahmishal shahmishal requested a review from parkera September 20, 2024 17:11
@parkera
Copy link
Contributor

parkera commented Sep 20, 2024

How much does this actually address the overall problem reported in #76555?

@finagolfin
Copy link
Member Author

I don't know. Without @MahdiBM trying the fixed snapshot builds I've specified there, I can't say if this is the fix.

@shahmishal
Copy link
Member

shahmishal commented Sep 20, 2024

I would prefer we get this into release/6.0 branch before pulling it into release/6.0.x branches. Also, it will be important to understand from @MahdiBM if the fix has been resolved in main.

@finagolfin
Copy link
Member Author

@shahmishal, the main commit was already merged into release/6.0 in #76504 recently. I simply added #76224 to this 6.0.1 pull because it may also help, and I'm submitting it to release/6.0 next anyway.

If you want, I can change this 6.0.1 pull to only include #76019, which is already in the 6.0 branch.

@finagolfin
Copy link
Member Author

added #76224 to this 6.0.1 pull because it may also help, and I'm submitting it to release/6.0 next anyway.

Submitted in #76613.

@MahdiBM
Copy link

MahdiBM commented Sep 20, 2024

On it 🙂

@MahdiBM
Copy link

MahdiBM commented Sep 20, 2024

@finagolfin trying on these:

- main-snapshot-2024-09-04
- main-snapshot-2024-08-29

Please let me know if these are NOT what you meant.

@finagolfin
Copy link
Member Author

That looks right: you are trying to test this commit, c069bd6, which it says there was first built into the Sep. 4 snapshot build of the toolchain.

@MahdiBM
Copy link

MahdiBM commented Sep 20, 2024

@finagolfin unfortunately i get:

error: upcoming feature 'StrictConcurrency' is already enabled as of Swift version 6

Because of a dependency.
Not sure how to bypass this. The package is a Swift 6 package with all targets in Swift 5 mode.

The CI File
name: test build

on:
  pull_request: { types: [opened, reopened, synchronize] }

concurrency:
  group: ${{ github.workflow }}-${{ github.ref }}
  cancel-in-progress: true

jobs:
  unit-tests:
    strategy:
      fail-fast: false
      matrix:
        snapshot:
          - main-snapshot-2024-09-04
          - main-snapshot-2024-08-29
        machine:
          - "medium" # 16gb 8cpu c7i-flex
          - "large" # 32gb 16cpu c7i-flex
          - "huge-stable-arm" # 128gb 64cpu bare metal c7g

    runs-on:
      labels:
        - runs-on
        - runner=${{ matrix.machine }}
        - run-id=${{ github.run_id }}

    timeout-minutes: 60

    steps:
      - name: Configure git
        run: |
          git config --global --add url."https://${{ secrets.GH_PAT }}@github.com/".insteadOf "https://github.com/"
          git config --global --add url."https://${{ secrets.GH_PAT }}@github.com/".insteadOf "git@github.com:"
          git config --global --add safe.directory '*'

      - name: Check out
        uses: actions/checkout@v4

      - name: Install jemalloc
        run: |
          sudo apt-get update -y
          sudo apt-get install -y libjemalloc-dev

      - uses: vapor/swiftly-action@v0.1
        with:
          toolchain: ${{ matrix.snapshot }}

      - name: Build ${{ matrix.snapshot }}
        run: |
          swift package update
          swift build --build-tests

EDIT:
Trying with swift build --build-tests -Xswiftc -continue-building-after-errors now.

@finagolfin
Copy link
Member Author

I haven't messed with the language mode settings myself. Alternately, you can try with the older July 19 and July 21 snapshot builds of the 6.0 toolchain that I mentioned first, as the former was before these new Foundation libraries was added.

@MahdiBM
Copy link

MahdiBM commented Sep 20, 2024

No luck.

          - 6.0-snapshot-2024-07-19
          - 6.0-snapshot-2024-07-21

Complains about CompilerPluginSupport or whatever

@bnbarham
Copy link
Contributor

Are you able to share your Package.swift? Not clear on where StrictConcurrency would be being enabled here.

@MahdiBM
Copy link

MahdiBM commented Sep 20, 2024

It's from pointfree's Dependency package which we depend on IIRC. Not our package.

@bnbarham
Copy link
Contributor

It's from pointfree's Dependency package which we depend on IIRC. Not our package.

Which one?

@MahdiBM
Copy link

MahdiBM commented Sep 20, 2024

@MahdiBM
Copy link

MahdiBM commented Sep 20, 2024

Perhaps my dependencies are out of date? trying with addition of swift package update.

@MahdiBM
Copy link

MahdiBM commented Sep 20, 2024

Ok yeah ... there was a recent release and our dependencies were not up to date (actually now I notice GitHub's Dependabot has stopped working on Swift 6?! need to report that).
After that there was also another problem in another dep which i manually fixed.

Now getting this:

.build/aarch64-unknown-linux-gnu/debug/CNIODarwin-tool.build/module.modulemap:1:8: error: redefinition of module 'CNIODarwin'
1 | module CNIODarwin {
  |        `- error: redefinition of module 'CNIODarwin'
2 |     umbrella header ".build/checkouts/swift-nio/Sources/CNIODarwin/include/CNIODarwin.h"
3 |     export *

.build/aarch64-unknown-linux-gnu/debug/CNIODarwin.build/module.modulemap:1:8: note: previously defined here
1 | module CNIODarwin {
  |        `- note: previously defined here
2 |     umbrella header ".build/checkouts/swift-nio/Sources/CNIODarwin/include/CNIODarwin.h"
3 |     export *

Of course none of these errors occur on the release Docker images.

EDIT: yeah can't get past this, building in release mode doesn't help either.

@MahdiBM
Copy link

MahdiBM commented Sep 20, 2024

FWIW The builds fail at almost the same time:

          - main-snapshot-2024-09-04
          - main-snapshot-2024-08-29

@MahdiBM
Copy link

MahdiBM commented Sep 24, 2024

For the record, this is the result of comparing 07-19 vs 07-21 swift 6.0 nightlies, which I mentioned to @finagolfin.
TL;DR: 30% of the regression appears to have happened between those snapshots.

I was unable to try the trunk snapshots considering they would result in weird build failures.

And also there seems to be some extra noble-specific regressions as well.

@finagolfin
Copy link
Member Author

6.0.1 was just tagged, so closing this. @MahdiBM, feel free to nominate these two trunk pulls for a subsequent patch release instead, if you can show they are causing a significant portion of your slowdown.

@finagolfin finagolfin closed this Sep 25, 2024
@finagolfin finagolfin deleted the release/6.0.0 branch October 3, 2024 14:54
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.

6 participants