Move function deletion from the stack to the heap. by resistor · Pull Request #11611 · pytorch/pytorch

resistor · 2018-09-12T23:53:01Z

This eliminates the need for any heuristics regarding stack size limits.

This is a re-do #11534 with a fix to properly handle cases where
multiple edges exist between a pair of functions.

resistor · 2018-09-12T23:53:26Z

Will rebase past the revert once it makes it to GitHub

resistor · 2018-09-12T23:53:45Z

Will do perf measurement soon

resistor · 2018-09-13T00:13:14Z

Rebased.

resistor · 2018-09-13T03:42:40Z

@pytorchbot retest this please

resistor · 2018-09-13T05:11:12Z

New performance results:

MASTER:
Time to build graph (s):
0.001078948974609375
Time to free graph (s):
0.00015817642211914064

Time to build graph (s):
0.0010588359832763672
Time to free graph (s):
0.00015884828567504884

WITH PR:
Time to build graph (s):
0.0009531970024108887
Time to free graph (s):
0.0001594715118408203

Time to build graph (s):
0.0009731245040893555
Time to free graph (s):
0.00015877246856689454

torch/csrc/autograd/function.cpp

apaszke · 2018-09-13T19:06:01Z

@resistor is it the small benchmark or the big benchmark?

resistor · 2018-09-13T19:56:32Z

The small one

…

On Sep 13, 2018, at 12:06 PM, Adam Paszke ***@***.***> wrote: @resistor is it the small benchmark or the big benchmark? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

ezyang · 2018-09-14T03:33:50Z

@pytorchbot retest this please

resistor · 2018-09-17T20:05:29Z

@pytorchbot retest this please

resistor · 2018-09-17T22:22:46Z

@apaszke Ready for another pass when you have a chance.

ezyang · 2018-09-18T04:48:35Z

has spurious submodule change again

resistor · 2018-09-18T05:28:25Z

has spurious submodule change again

Fixed now.

apaszke

I'm not sure why gatherFunctions looks like that. I thought of pushing the use_count check to still happen before we push anything onto the function stack.

torch/csrc/autograd/function.cpp

resistor · 2018-09-18T16:34:50Z

@apaszke I meant to do that (check use_count == 1).

torch/csrc/autograd/function.cpp

resistor · 2018-09-21T16:09:19Z

@apaszke @goldsborough OK to land?

goldsborough

facebook-github-bot

resistor is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

resistor has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

This eliminates the need for any heuristics regarding stack size limits. This is a re-do pytorch#11534 with a fix to properly handle cases where multiple edges exist between a pair of functions.

facebook-github-bot

resistor has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

pietern · 2019-07-17T12:21:27Z

torch/csrc/autograd/function.cpp

+  func->release_variables();
+
+  for (auto& edge : func->next_edges()) {
+    if (edge.function.use_count() == 1) {


@resistor Does this mean any function with a use_count > 1 is ignored?

resistor requested review from apaszke, colesbury, ezyang, gchanan, soumith and zdevito as code owners September 12, 2018 23:53

resistor force-pushed the deleter branch 2 times, most recently from 050b8ec to 44ba7b9 Compare September 13, 2018 00:12

resistor force-pushed the deleter branch 2 times, most recently from 03b2fe0 to eb854cb Compare September 13, 2018 05:10

resistor force-pushed the deleter branch from eb854cb to a2a158a Compare September 13, 2018 16:01

apaszke reviewed Sep 13, 2018

View reviewed changes

torch/csrc/autograd/function.cpp 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

resistor force-pushed the deleter branch from a2a158a to 1287f8b Compare September 13, 2018 21:12

resistor force-pushed the deleter branch from 1287f8b to 3b9e3f6 Compare September 16, 2018 06:33

resistor force-pushed the deleter branch from 3b9e3f6 to 06cbc56 Compare September 18, 2018 05:28

apaszke reviewed Sep 18, 2018

View reviewed changes

torch/csrc/autograd/function.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/autograd/function.cpp Outdated

This comment was marked as off-topic.

Sign in to view

resistor force-pushed the deleter branch from 06cbc56 to 17b9a80 Compare September 18, 2018 16:31

apaszke approved these changes Sep 18, 2018

View reviewed changes

torch/csrc/autograd/function.cpp Outdated

This comment was marked as off-topic.

Sign in to view

resistor force-pushed the deleter branch from 17b9a80 to 42e6a35 Compare September 19, 2018 06:26

apaszke reviewed Sep 19, 2018

View reviewed changes

resistor force-pushed the deleter branch from 42e6a35 to f30885c Compare September 19, 2018 16:22

apaszke reviewed Sep 19, 2018

View reviewed changes

torch/csrc/autograd/function.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

torch/csrc/autograd/function.cpp Outdated Show resolved Hide resolved

goldsborough approved these changes Sep 21, 2018

View reviewed changes

facebook-github-bot reviewed Sep 21, 2018

View reviewed changes

resistor force-pushed the deleter branch from f30885c to 6c5117d Compare September 21, 2018 17:08

facebook-github-bot reviewed Sep 21, 2018

View reviewed changes

Move function deletion from the stack to the heap.

1779145

This eliminates the need for any heuristics regarding stack size limits. This is a re-do pytorch#11534 with a fix to properly handle cases where multiple edges exist between a pair of functions.

resistor force-pushed the deleter branch from 6c5117d to 1779145 Compare September 21, 2018 17:19

facebook-github-bot reviewed Sep 21, 2018

View reviewed changes

apaszke approved these changes Sep 21, 2018

View reviewed changes

facebook-github-bot closed this in 89d56ae Sep 21, 2018

ezyang added the merged label Jun 26, 2019

pietern mentioned this pull request Jul 17, 2019

Loss explosion with DataParallel on WGAN models #19024

Closed

pietern reviewed Jul 17, 2019

View reviewed changes

Conversation

resistor commented Sep 12, 2018

Uh oh!

resistor commented Sep 12, 2018

Uh oh!

resistor commented Sep 12, 2018

Uh oh!

resistor commented Sep 13, 2018

Uh oh!

resistor commented Sep 13, 2018

Uh oh!

resistor commented Sep 13, 2018

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

apaszke commented Sep 13, 2018

Uh oh!

resistor commented Sep 13, 2018 via email

Uh oh!

ezyang commented Sep 14, 2018

Uh oh!

resistor commented Sep 17, 2018

Uh oh!

resistor commented Sep 17, 2018

Uh oh!

ezyang commented Sep 18, 2018

Uh oh!

resistor commented Sep 18, 2018

Uh oh!

apaszke left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

resistor commented Sep 18, 2018

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

Uh oh!

resistor commented Sep 21, 2018

Uh oh!

goldsborough left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

pietern Jul 17, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development