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

Cache ElementwiseKernel kernel globally instead of per instance #2474

Merged
merged 1 commit into from Nov 20, 2019
Merged

Cache ElementwiseKernel kernel globally instead of per instance #2474

merged 1 commit into from Nov 20, 2019

Conversation

niboshi
Copy link
Member

@niboshi niboshi commented Sep 17, 2019

Fixes #2469

@emcastillo emcastillo self-assigned this Sep 18, 2019
@emcastillo
Copy link
Member

Jenkins, test this please

emcastillo
emcastillo approved these changes Sep 18, 2019
View reviewed changes
Copy link
Member

@emcastillo emcastillo left a comment

Choose a reason for hiding this comment

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

LGTM

@emcastillo emcastillo added this to the v7.0.0b4 milestone Sep 18, 2019
@emcastillo emcastillo added cat:bug Bugs cat:enhancement Improvements to existing features and removed cat:bug Bugs labels Sep 18, 2019
@chainer-ci
Copy link
Member

Jenkins CI test (for commit 6b4df36, target branch master) failed with status FAILURE.

@emcastillo
Copy link
Member

emcastillo commented Sep 18, 2019 •
edited

Can you verify that the test failures are not PR related, please?

@niboshi
Copy link
Member Author

niboshi commented Sep 18, 2019

At least some of them seem related. I'll fix them.

@niboshi
Copy link
Member Author

niboshi commented Sep 19, 2019

I've made a fix, but it may degrade performance.
I'll make a measurement.

@niboshi
Copy link
Member Author

niboshi commented Sep 19, 2019 •
edited

Measured using cupy-perf.
No negative impact is observed.

Before this PR:

userkenel           :     8.661 us   +/- 0.294 (min:    8.378) us

This PR:

userkenel           :     8.514 us   +/- 0.345 (min:    8.265) us
  • perf_userkernel measures ElementwiseKernel calls repeatedly reusing a single instance.
  • Only showing CPU time.

emcastillo
emcastillo approved these changes Sep 19, 2019
View reviewed changes
Copy link
Member

@emcastillo emcastillo left a comment

Choose a reason for hiding this comment

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

LGTM

@emcastillo
Copy link
Member

Jenkins, test this please

@emcastillo emcastillo added the st:test-and-merge (deprecated) Ready to merge after test pass. label Sep 19, 2019
@chainer-ci
Copy link
Member

Jenkins CI test (for commit 127c303, target branch master) failed with status FAILURE.

@emcastillo
Copy link
Member

Jenkins, test this please

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 127c303, target branch master) failed with status FAILURE.

@niboshi
Copy link
Member Author

niboshi commented Sep 19, 2019

I'll check the failures.

@niboshi
Copy link
Member Author

niboshi commented Sep 20, 2019 •
edited

I once was able to replicate the failure 100% of the time, but now for some reason can't replicate it at all... even in docker with chainer-test.

@niboshi niboshi added cat:bug Bugs and removed cat:enhancement Improvements to existing features labels Sep 20, 2019
@chainer-ci
Copy link
Member

@emcastillo This pull-request is marked as st:test-and-merge, but there were no activities for the last 3 days. Could you check?

@niboshi
Copy link
Member Author

niboshi commented Sep 24, 2019

@emcastillo Can you retrigger Jenkins to check if the failure is persistent?

@emcastillo
Copy link
Member

Jenkins, test this please

@emcastillo emcastillo removed the st:test-and-merge (deprecated) Ready to merge after test pass. label Sep 24, 2019
@chainer-ci
Copy link
Member

Jenkins CI test (for commit 127c303, target branch master) failed with status FAILURE.

@asi1024 asi1024 removed this from the v7.0.0b4 milestone Sep 26, 2019
@kmaehashi
Copy link
Member

pfnCI, test this please.

@pfn-ci-bot
Copy link
Collaborator

Successfully created a job for commit 127c303:

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 127c303, target branch master) succeeded!

@emcastillo
Copy link
Member

Let me re-run jenkins to see if the failure before reproduces

@emcastillo
Copy link
Member

Jenkins, test this please

@pfn-ci-bot
Copy link
Collaborator

Successfully created a job for commit 127c303:

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 127c303, target branch master) succeeded!

@emcastillo
Copy link
Member

Lets merge, hope it wont blow up later 😅

@emcastillo emcastillo merged commit 9ad936b into cupy:master Nov 20, 2019
@emcastillo emcastillo added this to the v7.0.0 milestone Nov 20, 2019
@kmaehashi
Copy link
Member

Should it be backported as this is a bug? @niboshi @emcastillo

@niboshi niboshi deleted the elementwise-kernel-cache branch November 20, 2019 07:08
@niboshi
Copy link
Member Author

niboshi commented Nov 20, 2019

@kmaehashi Yes, let's do it.

@niboshi niboshi added the to-be-backported Pull-requests to be backported to stable branch label Nov 20, 2019
emcastillo pushed a commit to emcastillo/cupy that referenced this pull request Nov 20, 2019
Merge pull request cupy#2474 from niboshi/elementwise-kernel-cache
8fb4aad 
Cache `ElementwiseKernel` kernel globally instead of per instance
@emcastillo emcastillo mentioned this pull request Nov 20, 2019
Merged
@niboshi niboshi mentioned this pull request Nov 25, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emcastillo emcastillo emcastillo approved these changes

Assignees

@emcastillo emcastillo

Labels
cat:bug Bugs to-be-backported Pull-requests to be backported to stable branch
Projects
None yet
Milestone
v7.0.0
Development

Successfully merging this pull request may close these issues.

Newly created instances of cupy.ElementwiseKernel always synchronize
6 participants
@niboshi @emcastillo @chainer-ci @kmaehashi @pfn-ci-bot @asi1024