swift icon indicating copy to clipboard operation
swift copied to clipboard

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

Open finagolfin opened this issue 1 year ago • 19 comments

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: apple/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 avatar Sep 20 '24 16:09 finagolfin

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

shahmishal avatar Sep 20 '24 17:09 shahmishal

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

parkera avatar Sep 20 '24 17:09 parkera

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

finagolfin avatar Sep 20 '24 18:09 finagolfin

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.

shahmishal avatar Sep 20 '24 18:09 shahmishal

@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 avatar Sep 20 '24 18:09 finagolfin

added https://github.com/swiftlang/swift/pull/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.

finagolfin avatar Sep 20 '24 18:09 finagolfin

On it 🙂

MahdiBM avatar Sep 20 '24 19:09 MahdiBM

@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.

MahdiBM avatar Sep 20 '24 19:09 MahdiBM

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

finagolfin avatar Sep 20 '24 19:09 finagolfin

@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 "[email protected]:"
          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/[email protected]
        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.

MahdiBM avatar Sep 20 '24 19:09 MahdiBM

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.

finagolfin avatar Sep 20 '24 19:09 finagolfin

No luck.

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

Complains about CompilerPluginSupport or whatever

MahdiBM avatar Sep 20 '24 20:09 MahdiBM

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

bnbarham avatar Sep 20 '24 20:09 bnbarham

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

MahdiBM avatar Sep 20 '24 20:09 MahdiBM

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

Which one?

bnbarham avatar Sep 20 '24 20:09 bnbarham

https://github.com/pointfreeco/swift-dependencies

MahdiBM avatar Sep 20 '24 21:09 MahdiBM

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

MahdiBM avatar Sep 20 '24 21:09 MahdiBM

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 avatar Sep 20 '24 21:09 MahdiBM

FWIW The builds fail at almost the same time:

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

MahdiBM avatar Sep 20 '24 23:09 MahdiBM

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.

MahdiBM avatar Sep 24 '24 20:09 MahdiBM

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 avatar Sep 25 '24 06:09 finagolfin