Cache cufft plans #8344

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

Merged

ssnl merged 20 commits into pytorch:master from ssnl:cache_fft

Jun 22, 2018

Collaborator

ssnl commented Jun 11, 2018 •

edited

Loading

#8120
Empirical speed up when running our test 10 times is 9.57s => 7.90s with cache enabled.

ssnl requested review from apaszke, colesbury, ezyang, gchanan, soumith and zdevito as code owners

June 11, 2018 20:20

ssnl changed the title ~~Cache cufft plans~~ [wip] Cache cufft plans

ssnl force-pushed the cache_fft branch 4 times, most recently from 85dd24d to fa76ac4 Compare

June 11, 2018 22:27

Collaborator Author

ssnl commented Jun 12, 2018

@pytorchbot retest this please

1 similar comment

Collaborator Author

ssnl commented Jun 12, 2018

@pytorchbot retest this please

ezyang reviewed

View reviewed changes

aten/src/ATen/Context.h Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ezyang reviewed

View reviewed changes

torch/_torch_docs.py Outdated

This comment was marked as off-topic.

Sign in to view

ezyang reviewed

View reviewed changes

torch/_torch_docs.py Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ssnl changed the title ~~[wip] Cache cufft plans~~ Cache cufft plans

ezyang reviewed

View reviewed changes

aten/src/ATen/third_party/ParamsMap.h Outdated

This comment was marked as off-topic.

Sign in to view

ezyang reviewed

View reviewed changes

aten/src/ATen/native/cuda/CuFFTUtils.h Outdated

This comment was marked as off-topic.

Sign in to view

ezyang approved these changes

View reviewed changes

Contributor

ezyang left a comment

I'm going to approve this, but I am a little concerned about the discoverability of the caching mechanism. Should we perhaps figure out some way to turn the caching on by default, e.g., setup some sort of eviction policy in the cache?

ssnl force-pushed the cache_fft branch 2 times, most recently from f50c077 to 9ab9994 Compare

June 18, 2018 16:57

Collaborator Author

ssnl commented Jun 18, 2018

Now using an LRU cache.

docs are not updated yet. I want to check that CI passes first.

ssnl force-pushed the cache_fft branch 3 times, most recently from 7cf1e75 to 4a4c66d Compare

June 18, 2018 21:21

Collaborator Author

ssnl commented Jun 19, 2018

:D CUDA hook worked pretty well. Will update the docs soon.

ssnl force-pushed the cache_fft branch from 4a4c66d to b43707d Compare

June 19, 2018 16:10

ssnl mentioned this pull request

Add torch.backends.cudnn.is_available #8703

Merged

ssnl commented

View reviewed changes

aten/src/ATen/function_wrapper.py Outdated

This comment was marked as off-topic.

Sign in to view

ssnl and others added 16 commits

June 22, 2018 10:23


          suffix CuFFTParams members with _

8a9fa9c


          import print_function for py2


          lint


          fix potential race; add dummy impl for CPU only builds

34aaf76


          cpp formatting; remove nccl makefile change

afd3cac


          Use CUDA hooks instead


          comments and doc

05fb038


          update the error message

6e21f02


          move LRU cachae to a separate file and native::detail namespace

b2a7447


          update comment

1091ee3


          specify NOTE location in CuFFTPlanCache.h

ad8ddb0


          update disabled_features.yaml to make amd ci work

54bd1fc


          another fix for AMD CI in disabled_features.yaml

443e28c


          Wrap cufft_plan_cache_* methods in __HIP_PLATFORM_HCC__

a234ac8


          improve the notes

e4bf4a9


          lint

940ceac

ssnl force-pushed the cache_fft branch from d980a22 to 940ceac Compare

June 22, 2018 14:25

ssnl requested a review from orionr as a code owner

June 22, 2018 14:25

orionr reviewed

View reviewed changes

Contributor

orionr left a comment

Is the third_party/onnx change actually required for this PR?


          revert onnx change

fa5398c

Collaborator Author

ssnl commented Jun 22, 2018

@orionr reverted that change

ezyang approved these changes

View reviewed changes


          put back inlining for CUFFT_CHECK

98ecb38

Collaborator Author

ssnl commented Jun 22, 2018

ci failure looks unrelated

ssnl merged commit e6c7b38 into pytorch:master

ssnl deleted the cache_fft branch

June 22, 2018 17:02

ssnl mentioned this pull request

[PyTorch] Cache CUFFT plans in fft #8120

Closed

ssnl restored the cache_fft branch

June 22, 2018 19:01

ssnl deleted the cache_fft branch

June 22, 2018 19:01

ezyang added the open source label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

orionr orionr left review comments

ezyang ezyang approved these changes

apaszke Awaiting requested review from apaszke

colesbury Awaiting requested review from colesbury

gchanan Awaiting requested review from gchanan

soumith Awaiting requested review from soumith

zdevito Awaiting requested review from zdevito

Labels