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.

Sign up for GitHub

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

Jump to bottom

Delay-load ShellExecute #67442

Closed
zooba opened this issue Jan 16, 2015 · 7 comments
Closed

Delay-load ShellExecute #67442

zooba opened this issue Jan 16, 2015 · 7 comments
Assignees
@zooba
Labels
OS-windows type-feature A feature request or enhancement

Comments

@zooba
Copy link
Member

zooba commented Jan 16, 2015

BPO 23253
Nosy @brettcannon, @tjguk, @zware, @zooba
Files
  • 23253.diff
    		•

    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 2015-01-24.16:54:29.003>
created_at = <Date 2015-01-16.23:11:50.592>
labels = ['type-feature', 'OS-windows']
title = 'Delay-load ShellExecute'
updated_at = <Date 2015-01-24.16:54:29.000>
user = 'https://github.com/zooba'

    bugs.python.org fields:

    activity = <Date 2015-01-24.16:54:29.000>
actor = 'python-dev'
assignee = 'steve.dower'
closed = True
closed_date = <Date 2015-01-24.16:54:29.003>
closer = 'python-dev'
components = ['Windows']
creation = <Date 2015-01-16.23:11:50.592>
creator = 'steve.dower'
dependencies = []
files = ['37743']
hgrepos = []
issue_num = 23253
keywords = ['patch']
message_count = 7.0
messages = ['234151', '234183', '234199', '234201', '234255', '234621', '234622']
nosy_count = 5.0
nosy_names = ['brett.cannon', 'tim.golden', 'python-dev', 'zach.ware', 'steve.dower']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue23253'
versions = ['Python 3.5']
    @zooba
    Copy link
    Member Author

    zooba commented Jan 16, 2015

    Currently, pythonXY.dll has a dependency on shell32.dll solely for the os.startfile (Modules/posixmodule.c) function. This is quite a heavy dependency that many would rather not have to load (e.g. lightweight server configurations).

    It would be nice to delay load the DLL and fail the operation if it is not available.

    (This is as much a reminder for myself as anything else, but if someone wants to do it then feel free.)

    @zooba zooba added OS-windows type-feature A feature request or enhancement labels Jan 16, 2015
    @zooba
    Copy link
    Member Author

    zooba commented Jan 17, 2015

    Attached a patch.

    Comparing the time for "python.exe -c '0'" with Powershell's Measure-Command tool, it looks like there's a 3-4ms (~8-10%) improvement in startup time too. That's not at all robust, but it's certainly no worse. (I'm not surprised - shell32.dll is a horrendously big dependency and we're better off without it.)

    @zooba zooba self-assigned this Jan 17, 2015
    @tjguk
    Copy link
    Member

    tjguk commented Jan 17, 2015

    I'm +0.75. I think the idea's fine in principle and the patch (by
    inspection) seems to do the right things.

    My only concerns are: that posixmodule.c becomes even longer and more
    involved; and that the benefit might not quite be great enough to
    justify the added complexity.

    @tjguk tjguk changed the title Delay-load ShellExecute[AW] in os.startfile Delay-load ShellExecute Jan 17, 2015
    @zooba
    Copy link
    Member Author

    zooba commented Jan 17, 2015

    Yeah, I hate touching posixmodule.c for the same reason. It'd be nice to split it up into separate platform files, but nobody is volunteering for that.

    If you focus on the performance, then yeah, this change probably isn't worth it. OTOH, the number of Windows platforms keep increasing (e.g. ARM, phone/tablet/various sandboxes, etc.) and the fewer dependencies we have the more likely Python will Just Work in them. (And the more value that a split up posixmodule.c will have... guess it'll have to happen eventually.)

    @brettcannon
    Copy link
    Member

    If you want a robust measurement of startup impact, the benchmark suite has two benchmarks specifically for startup (w/ and w/o site.py).

    @zooba
    Copy link
    Member Author

    zooba commented Jan 24, 2015

    I assume you're referring to normal_startup and startup_nosite in perf.py at h.p.o/benchmarks? Handy to know about (I need to explore our top-level repos more often, obviously), but probably still not going to measure time in the Windows PE loader as accurately as it'd need to be to conclusively prove a speed advantage. I'd probably need to hit up the Windows team for some of their profiling tools to get good numbers here.

    Still, it's indisputable that this change will reduce the initial memory overhead, so I'll take Tim's 0.75 and run with it :)

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 24, 2015

    New changeset 5bff604a864e by Steve Dower in branch 'default':
    Closes bpo-23253: Delay-load ShellExecute
    https://hg.python.org/cpython/rev/5bff604a864e

    @python-dev python-dev mannequin closed this as completed Jan 24, 2015
    @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
    Assignees

    @zooba zooba

    Labels
    OS-windows type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @brettcannon @tjguk @zooba