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

Remove shlwapi dependency on Windows #89883

Closed
zooba opened this issue Nov 5, 2021 · 6 comments
Closed

Remove shlwapi dependency on Windows #89883

zooba opened this issue Nov 5, 2021 · 6 comments
Assignees
Labels
3.9 only security fixes 3.10 only security fixes 3.11 only security fixes OS-windows performance Performance or resource usage

Comments

@zooba
Copy link
Member

zooba commented Nov 5, 2021

BPO 45720
Nosy @pfmoore, @tjguk, @zware, @eryksun, @zooba, @miss-islington
PRs
  • bpo-45720: Drop references to shlwapi #29417
  • [3.10] bpo-45720: Drop references to shlwapi.dll on Windows (GH-29417) #29435
  • [3.9] bpo-45720: Drop references to shlwapi.dll on Windows (GH-29417) #29436
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/zooba'
    closed_at = <Date 2021-11-06.01:39:50.499>
    created_at = <Date 2021-11-05.01:04:25.416>
    labels = ['3.10', '3.11', '3.9', 'OS-windows', 'performance']
    title = 'Remove shlwapi dependency on Windows'
    updated_at = <Date 2021-11-06.01:39:50.499>
    user = 'https://github.com/zooba'

    bugs.python.org fields:

    activity = <Date 2021-11-06.01:39:50.499>
    actor = 'steve.dower'
    assignee = 'steve.dower'
    closed = True
    closed_date = <Date 2021-11-06.01:39:50.499>
    closer = 'steve.dower'
    components = ['Windows']
    creation = <Date 2021-11-05.01:04:25.416>
    creator = 'steve.dower'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 45720
    keywords = ['patch']
    message_count = 6.0
    messages = ['405762', '405763', '405831', '405833', '405836', '405840']
    nosy_count = 6.0
    nosy_names = ['paul.moore', 'tim.golden', 'zach.ware', 'eryksun', 'steve.dower', 'miss-islington']
    pr_nums = ['29417', '29435', '29436']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'performance'
    url = 'https://bugs.python.org/issue45720'
    versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

    @zooba
    Copy link
    Member Author

    zooba commented Nov 5, 2021

    According to https://twitter.com/BruceDawson0xB/status/1455714820485894151?s=20 there are some serious performance implications from referencing shlwapi.dll.

    It turns out, we only use it for one native path calculation function, which is easily replaceable (since Windows 8). So we can drop the dependency and get a startup (and shutdown) improvement back to 3.9.

    @zooba zooba added 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes labels Nov 5, 2021
    @zooba zooba self-assigned this Nov 5, 2021
    @zooba zooba added OS-windows 3.9 only security fixes performance Performance or resource usage 3.10 only security fixes 3.11 only security fixes labels Nov 5, 2021
    @zooba zooba self-assigned this Nov 5, 2021
    @zooba zooba added OS-windows performance Performance or resource usage labels Nov 5, 2021
    @zooba
    Copy link
    Member Author

    zooba commented Nov 5, 2021

    FTR, I see about 1-1.5ms improvement (using timeit to do a check_call) out of ~32ms total startup time.

    I also don't actually see gdi32 being transitively loaded as claimed in the Twitter thread, even back to 3.8. So presumably there's something else going on to cause that issue.

    Either way, this is an easy change with non-zero benefit (at the very least, it gets rid of a dependency we are better off avoiding these days).

    @eryksun
    Copy link
    Contributor

    eryksun commented Nov 5, 2021

    I also don't actually see gdi32 being transitively loaded as
    claimed in the Twitter thread, even back to 3.8. So presumably
    there's something else going on to cause that issue.

    Since Windows XP, shlwapi.dll has increasingly made use of delay loading, but up to Windows 8.1 it was still immediately loading user32.dll and gdi32.dll. In Windows 10, shlwapi.dll finally made them delay loaded dependencies. Also, in Windows 10, PathIsRelativeW is forwarded to the implementation in kernelbase.dll, not that it matters from the end user's perspective since it's not in an API set.

    Regarding user32.dll, there's still an issue with ssl, hashlib, winsound, and ctypes not delay loading it, or DLLs that directly depend on it such as ole32.dll and oleaut32.dll. In most use cases these DLLs would never be loaded.

    When user32.dll loads, the process and the loading thread are converted to GUI versions, i.e. they're extended in the kernel with the window manager's Win32Process and Win32Thread structures, and the process connects to a window station and desktop (e.g. r"WinSta0\Default"). One concern with converting to a GUI process is that the system doesn't send CTRL_LOGOFF_EVENT and CTRL_SHUTDOWN_EVENT console events to GUI processes. Thus a console script can't handle those events with a simple ctypes-based handler as long as importing ctypes always loads user32.dll.

    @zooba
    Copy link
    Member Author

    zooba commented Nov 5, 2021

    New changeset a4774f4 by Steve Dower in branch 'main':
    bpo-45720: Drop references to shlwapi.dll on Windows (GH-29417)
    a4774f4

    @zooba
    Copy link
    Member Author

    zooba commented Nov 6, 2021

    New changeset 804ea41 by Steve Dower in branch '3.10':
    bpo-45720: Drop references to shlwapi.dll on Windows (GH-29417)
    804ea41

    @zooba
    Copy link
    Member Author

    zooba commented Nov 6, 2021

    New changeset 5017306 by Miss Islington (bot) in branch '3.9':
    bpo-45720: Drop references to shlwapi.dll on Windows (GH-29417)
    5017306

    @zooba zooba closed this as completed Nov 6, 2021
    @zooba zooba closed this as completed Nov 6, 2021
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 only security fixes 3.10 only security fixes 3.11 only security fixes OS-windows performance Performance or resource usage
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants