Skip to content

[🍒 stable/20240723] DynamicLoaderDarwin load images in parallel with preload #9445

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

Conversation

DmT021
Copy link

@DmT021 DmT021 commented Oct 18, 2024

Cherry-pick llvm#110646

This change enables DynamicLoaderDarwin to load modules in parallel using the thread pool. This new behavior is controlled by a new setting plugin.dynamic-loader.darwin.experimental.enable-parallel-image-load, which is enabled by default. When disabled, DynamicLoaderDarwin will load modules sequentially as before.

@DmT021 DmT021 marked this pull request as ready for review October 18, 2024 10:32
@DmT021 DmT021 requested a review from JDevlieghere as a code owner October 18, 2024 10:32
@DmT021
Copy link
Author

DmT021 commented Oct 18, 2024

@augusto2112 PTAL

@augusto2112
Copy link

@swift-ci test

@augusto2112
Copy link

@swift-ci test

@augusto2112
Copy link

@DmT021 the test suite failed on test:

FAIL: test_dsym (TestSwiftObjCMainConflictingDylibs.TestSwiftObjCMainConflictingDylibs)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/ec2-user/jenkins/workspace/apple-llvm-project-pr-macos/branch-stable/20240723/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1768, in test_method
    return attrvalue(self)
  File "/Users/ec2-user/jenkins/workspace/apple-llvm-project-pr-macos/branch-stable/20240723/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 149, in wrapper
    return func(*args, **kwargs)
  File "/Users/ec2-user/jenkins/workspace/apple-llvm-project-pr-macos/branch-stable/20240723/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 149, in wrapper
    return func(*args, **kwargs)
  File "/Users/ec2-user/jenkins/workspace/apple-llvm-project-pr-macos/branch-stable/20240723/llvm-project/lldb/test/API/lang/swift/clangimporter/objcmain_conflicting_dylibs/TestSwiftObjCMainConflictingDylibs.py", line 39, in test
    self.expect("fr var baz", "correct baz", substrs=["i_am_from_Foo"])
  File "/Users/ec2-user/jenkins/workspace/apple-llvm-project-pr-macos/branch-stable/20240723/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2474, in expect
    self.fail(log_msg)
AssertionError: Ran command:
"fr var baz"

Got output:
(Baz) baz = {
  i_am_from_Bar = 23
}

Expecting sub string: "i_am_from_Foo" (was not found)
correct baz

I wonder if this could be a race condition? I'm going to rerun the test suite, but perhaps you could run this test locally on a loop to see if any run fails?

@augusto2112
Copy link

@swift-ci test macOS

@DmT021
Copy link
Author

DmT021 commented Oct 22, 2024

Yeah, I'm checking it.

EDIT: I can confirm there's a race condition somewhere. Will dig into it now.

@DmT021
Copy link
Author

DmT021 commented Oct 22, 2024

@augusto2112 The issue is that this test depends on the order of loading the dylibs. It depends on:

  1. the order of their appearance in the main executable.
otool -L /Users/dmt021/w/swift-project/build/Ninja-RelWithDebInfoAssert/lldb-macosx-arm64/lldb-test-build.noindex/lang/swift/clangimporter/objcmain_conflicting_dylibs/TestSwiftObjCMainConflictingDylibs.test_dwarf/a.out
/Users/dmt021/w/swift-project/build/Ninja-RelWithDebInfoAssert/lldb-macosx-arm64/lldb-test-build.noindex/lang/swift/clangimporter/objcmain_conflicting_dylibs/TestSwiftObjCMainConflictingDylibs.test_dwarf/a.out:
  @executable_path/libFoo.dylib (compatibility version 0.0.0, current version 0.0.0)
  @executable_path/libBar.dylib (compatibility version 0.0.0, current version 0.0.0)
  1. The order in which we add the modules in Target::GetOrCreateModule. When we load the modules in parallel, their order is random. That's why this test is flaky.

The issue can be reproduced without the patch if you swap -lFoo -lBar in lldb/test/API/lang/swift/clangimporter/objcmain_conflicting_dylibs/Makefile. When libBar is in front of libFoo LLDB will always resolve Buz incorrectly.

You can also see later in the tests we expect fr var baz in the context of Bar.swift to be resolved incorrectly:

        # This is failing because the Target-SwiftASTContext uses the
        # amalgamated target header search options from all dylibs.
        self.expect("expression baz", "wrong baz", substrs=["i_am_from_Foo"])
        self.expect("fr var baz", "wrong baz", substrs=["i_am_from_Foo"])

I think the best solution would be to fix this dependency on the order of modules in the ModuleList but not sure how difficult that would be.

@augusto2112
Copy link

@augusto2112 The issue is that this test depends on the order of loading the dylibs. It depends on:

  1. the order of their appearance in the main executable.
otool -L /Users/dmt021/w/swift-project/build/Ninja-RelWithDebInfoAssert/lldb-macosx-arm64/lldb-test-build.noindex/lang/swift/clangimporter/objcmain_conflicting_dylibs/TestSwiftObjCMainConflictingDylibs.test_dwarf/a.out
/Users/dmt021/w/swift-project/build/Ninja-RelWithDebInfoAssert/lldb-macosx-arm64/lldb-test-build.noindex/lang/swift/clangimporter/objcmain_conflicting_dylibs/TestSwiftObjCMainConflictingDylibs.test_dwarf/a.out:
  @executable_path/libFoo.dylib (compatibility version 0.0.0, current version 0.0.0)
  @executable_path/libBar.dylib (compatibility version 0.0.0, current version 0.0.0)
  1. The order in which we add the modules in Target::GetOrCreateModule. When we load the modules in parallel, their order is random. That's why this test is flaky.

The issue can be reproduced without the patch if you swap -lFoo -lBar in lldb/test/API/lang/swift/clangimporter/objcmain_conflicting_dylibs/Makefile. When libBar is in front of libFoo LLDB will always resolve Buz incorrectly.

You can also see later in the tests we expect fr var baz in the context of Bar.swift to be resolved incorrectly:

        # This is failing because the Target-SwiftASTContext uses the
        # amalgamated target header search options from all dylibs.
        self.expect("expression baz", "wrong baz", substrs=["i_am_from_Foo"])
        self.expect("fr var baz", "wrong baz", substrs=["i_am_from_Foo"])

I think the best solution would be to fix this dependency on the order of modules in the ModuleList but not sure how difficult that would be.

Besides the first binary, which should be the executable in the module list, the order of all the other binaries shouldn't matter. Also we probably should remove this test now that we no longer have a global scratch context.

Would you mind adding a new commit on top of the current one removing that test?

@DmT021
Copy link
Author

DmT021 commented Oct 22, 2024

Sure

This change enables `DynamicLoaderDarwin` to load modules in parallel
using the thread pool. This new behavior is controlled by a new setting
`plugin.dynamic-loader.darwin.experimental.enable-parallel-image-load`,
which is enabled by default. When disabled, DynamicLoaderDarwin will
load modules sequentially as before.
@DmT021 DmT021 force-pushed the 🍒/DynamicLoaderDarwin_parallel_image_load branch from c817472 to feb280d Compare October 23, 2024 23:42
@DmT021
Copy link
Author

DmT021 commented Oct 24, 2024

@augusto2112 rerun the tests, please

@augusto2112
Copy link

@swift-ci test

@augusto2112 augusto2112 merged commit 0daa5cc into swiftlang:stable/20240723 Oct 25, 2024
2 of 3 checks passed
@DmT021
Copy link
Author

DmT021 commented Oct 25, 2024

@augusto2112 thank you!

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