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

dask pyperformance benchmark hangs on win32 #118715

Closed
mdboom opened this issue May 7, 2024 · 7 comments
Closed

dask pyperformance benchmark hangs on win32 #118715

mdboom opened this issue May 7, 2024 · 7 comments
Labels
type-bug An unexpected behavior, bug, or error

Comments

@mdboom
Copy link
Contributor

mdboom commented May 7, 2024

Bug report

Bug description:

The dask benchmark in pyperformance hangs indefinitely, only on 32-bit Windows (not any other Tier 1 platform, as far as I can tell).

git bisect has narrowed it down to this commit 1d95451:

1d95451be1f3080904c00cc4c4a6cc519efdf321 is the first bad commit                                                 Author: Victor Stinner <vstinner@python.org>
Date:   Mon Mar 18 17:13:01 2024 +0100
gh-63207: Use GetSystemTimePreciseAsFileTime() in time.time() (#116822)
Doc/whatsnew/3.13.rst                              |  6 ++++
.../2024-03-14-17-21-25.gh-issue-63207.LV16SL.rst  |  4 +++
Python/pytime.c                                    | 38 +++++++++++++---------
3 files changed, 32 insertions(+), 16 deletions(-)
create mode 100644 Misc/NEWS.d/next/Library/2024-03-14-17-21-25.gh-issue-63207.LV16SL.rst

Cc @vstinner

To reproduce:

# Make a venv to install dependencies in
cpython\PCBuild\win32\python.exe -m venv venv
copy cpython\PCBuild\win32\python313.lib cpython
venv\Scripts\activate
python -m pip install pyperformance
python -m pyperformance run -b dask

I'm working on finding a more self contained reproducer, but may not have time for that immediately, so I thought I would report this in the meantime. I suspect it is an integer sizing mismatch bug in that commit, given that it doesn't happen on 64-bit Windows.

Link to earlier discussion

CPython versions tested on:

CPython main branch

Operating systems tested on:

Windows

@mdboom mdboom added the type-bug An unexpected behavior, bug, or error label May 7, 2024
@vstinner
Copy link
Member

vstinner commented May 7, 2024

benchmark hangs

Do you mean that it uses all the CPU, or that the process hangs and do nothing?

Can you check which lines are executed when it "hangs"?

@vstinner
Copy link
Member

vstinner commented May 7, 2024

I reproduced the issue. The problem comes from _WindowsTime of distributed/metrics.py.

Combine time.time() or time.monotonic() with time.perf_counter() to get an absolute clock with fine resolution.

https://github.com/dask/distributed/blob/d5edb4e536c9acc4998f803aacc0dd0188af21ce/distributed/metrics.py#L45-L98

@vstinner
Copy link
Member

vstinner commented May 7, 2024

I reported the issue to the dask/distributed project and I proposed a fix:

@vstinner
Copy link
Member

vstinner commented May 7, 2024

@mdboom: I suggest to close this issue since it's not a bug in Python, and it's now tracked in dask/distributed.

@mdboom mdboom closed this as completed May 7, 2024
@terryjreedy terryjreedy closed this as not planned Won't fix, can't repro, duplicate, stale May 7, 2024
@mdboom
Copy link
Contributor Author

mdboom commented May 9, 2024

I want to bring a discussion elsewhere back here.

@vstinner: I definitely have no experience with these Windows APIs, however, it seems that the change in 1d95451 introduced some stateful interaction between time.time and time.perf_counter, since they both now call py_win_perf_counter_frequency with the same global variable as base. Is this a regression for any code that calls time.time and time.perf_counter interleaved? What are the implications of that?

At a minimum, I think we should improve the "What's new" entry about this so people are aware of the interaction.

@vstinner
Copy link
Member

the change in 1d95451 introduced some stateful interaction between time.time and time.perf_counter, since they both now call py_win_perf_counter_frequency with the same global variable as base.

  • time.time() now calls GetSystemTimePreciseAsFileTime()
  • time.perf_counter() is unchanged
  • time.monotonic() now calls GetSystemTimePreciseAsFileTime().

It's documented at: https://docs.python.org/dev/whatsnew/3.13.html#time

@mdboom
Copy link
Contributor Author

mdboom commented May 10, 2024

Yes, but why did that break dask? Because the assumption about time.time having low resolution no longer holds? I think the concern is for possibly breaking other libraries using these APIs.

Sorry, I see you answered my question elsewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

3 participants