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

Add helpers to measure execution times #2740

Merged
merged 5 commits into from
Dec 12, 2019
Merged

Add helpers to measure execution times #2740

merged 5 commits into from
Dec 12, 2019

Conversation

asi1024
Copy link
Member

@asi1024 asi1024 commented Dec 2, 2019

I want to officially support some helpers to measure CPU/GPU execution times.

CuPy organization has cupy-benchmark currently, but it is for the maintainers to show users the performance results. I want to support this feature for also external contributors and users.

Copy link
Member

@leofang leofang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few quick comments...

cupy/testing/time.py Outdated Show resolved Hide resolved
func(*args)

for i in range(n):
ev1.synchronize()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need this synchronization.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GPU time before function entry may be counted if this synchronization is missing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only the ongoing work in the warmup loop might be counted in this case. How about moving this sync outside the current loop and putting it right after the warmup loop?

@asi1024 asi1024 changed the title [RFC] Add helpers to measure execution times Add helpers to measure execution times Dec 3, 2019
@asi1024 asi1024 marked this pull request as ready for review December 3, 2019 10:35
cupyx/time.py Outdated Show resolved Hide resolved
@asi1024
Copy link
Member Author

asi1024 commented Dec 10, 2019

I will support context manager interface for measurement of time in another PR.
@kmaehashi Could you take a look?

@kmaehashi kmaehashi added the cat:feature New features/APIs label Dec 11, 2019
cupyx/time.py Outdated
func(*args)

for i in range(n):
ev1.synchronize()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have to record ev1 first (or synchronize the entire device instead of ev1) to avoid effect from warmup.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? I think we should not record ev1 until all GPU threads finish computations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding of CUDA event semantics is that:

  • ev1.record() queues the event to the current stream
  • ev1.synchronize() blocks until the event completes

https://devblogs.nvidia.com/how-implement-performance-metrics-cuda-cc/

cupyx/__init__.py Outdated Show resolved Hide resolved
cupyx/time.py Outdated Show resolved Hide resolved
@asi1024
Copy link
Member Author

asi1024 commented Dec 11, 2019

PTAL.

@kmaehashi
Copy link
Member

pfnCI, test this please.

@pfn-ci-bot
Copy link
Collaborator

Successfully created a job for commit 36e9058:

Copy link
Member

@kmaehashi kmaehashi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 36e9058, target branch master) succeeded!

@kmaehashi kmaehashi merged commit bd62a31 into cupy:master Dec 12, 2019
@kmaehashi kmaehashi added this to the v8.0.0a1 milestone Dec 12, 2019
@asi1024 asi1024 deleted the time branch December 12, 2019 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:feature New features/APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants