Add mutex for CPU RNG and move TH to C++ #4041

yf225 · 2017-12-05T21:55:39Z

This fixes #3794.

Manually tested and there is no perf regression coming from the change.

colesbury · 2017-12-05T22:01:49Z

This will break the Windows build and is not exception safe. We should use std::mutex from C++11 instead of pthread mutexes. That will probably require changing at least THTensorRandom.c to C++.

colesbury · 2017-12-05T22:11:54Z

To clarify on the exception issue: a lot of functions in TH can call THError, including the TH_TENSOR_APPLY macros. In PyTorch, THError throws a C++ exception. If that happens while the pthread lock is held, it would not be released. That would lead to deadlocks if the random function were called again.

I suspect that moving all of TH to C++ would be a pain. You can avoid the C++ types in the THRandom.h header by using an opaque struct in the header file.

soumith

see @colesbury 's comments

soumith · 2018-01-23T15:11:00Z

@yf225 can you finish this this week, i think folks are blocked on it.

yf225 · 2018-01-23T17:40:16Z

@colesbury What do we think of using different generators for multi-threads (as @Stonesjtu suggested in #3794 (comment))? Perf-wise it might be faster than using mutex because we are not waiting for the lock at all, not sure about implementation yet.

soumith · 2018-01-23T18:18:11Z

@yf225 the PR corresponding to that was closed. We should just finish this as it currently stands and merge.

apaszke · 2018-01-23T18:23:02Z

I think thread local generators are not a good idea because it’s easy to forget to seed a worker thread

aten/src/TH/THRandom.cpp

yf225 · 2018-01-24T04:54:04Z

@colesbury It seems that if I do it as an opaque struct pointer and use new to initialize the struct in the THRandom.cpp file, then I am not sure if I should call delete on this pointer in THGenerator_free, because this THGenerator can be copied elsewhere and deleting the pointer here will destroy the mutex reference in those copies.

I guess moving TH to C++ land might be a cleaner way to go, although the code change would be quite big.

ezyang · 2018-01-24T05:06:34Z

Another issue is that if you want to actually acquire the mutex from C you are going to have to work hard to get unwinding to actually free it when necessary.

aten/src/TH/THRandom.cpp

colesbury · 2018-01-24T18:06:54Z

This still has the issue that it's not exception safe. An exception thrown after the lock call will not release the lock, causing deadlocks the next time a random function is called.

We should covert the C files to C++ and use std::lock_guard.

colesbury · 2018-01-24T18:07:50Z

We don't need to convert all of TH in one go. I think converting THTensorRandom.c to .cpp should be sufficient.

yf225 · 2018-01-24T18:14:11Z

@colesbury I think if we put the std::mutex in THRandom.h, then any file that includes this header would need to be converted to C++ as well?

yf225 · 2018-01-24T19:15:38Z

@colesbury If we change THTensorRandom.c to .cpp, we probably also need to change THTensor.c (that contains #include "generic/THTensorRandom.c") to .cpp, and that will require us to change many of other dependent TH files to C++ as well.

Haven't figured out what's the appropriate solution to this yet, or if adding try-catch block to catch THError would be a feasible approach.

colesbury · 2018-01-24T19:16:26Z

@yf225, yes. I think your current trick in THRandom.cpp can work. Move the definition of mutex_s to a separate header file that's included by THRandom.cpp and THTensorRandom.cpp. THRandom.h can keep the opaque definition.

As Adam Paszke points out, you can also simplify the definition of mutex_s a bit.

colesbury · 2018-01-24T19:17:32Z

I don't think a try-catch block is a good solution. We should be moving towards using C++, even if it requires more work.

yf225 · 2018-01-25T00:59:00Z

After changing things to C++, https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/test/scalar_tensor_test.cpp#L214 seems to fail when lhs is empty and rhs is a tensor with single element 0.

The error message is:

terminate called after throwing an instance of 'std::runtime_error'
 what():  invalid argument 2: out of range: 0 out of 0 at /private/home/willfeng/pytorch/aten/src/TH/generic/THTensorMath.cpp:382
../aten/tools/run_tests.sh: line 14: 23523 Aborted                 $BUILD_ROOT/src/ATen/test/scalar_tensor_test

In the normal case, TRY_CATCH_ELSE is expected to catch this std::runtime_error thrown from THTensorMath.cpp, but it wasn't the case here. I manually tested it and it seems that the catch block was not entered at all.

Any suggestions on how I should go about debugging this?

EDIT: fixed in #4857.

yf225 · 2018-01-26T18:50:02Z

There are a lot of unused variable warnings in TH after moving things to C++. I can fix them here or submit another PR for it.

yf225 · 2018-01-29T17:35:30Z

@colesbury @apaszke Wondering is there anything else I should do for this PR?

aten/src/TH/THRandom.cpp

  self->left = 1;
  self->seeded = 0;
  self->normal_is_valid = 0;
+  self->mutex = new std::mutex();


aten/src/TH/generic/THTensorRandom.cpp


 void THTensor_(geometric)(THTensor *self, THGenerator *_generator, double p)
 {
+  std::lock_guard<std::mutex> lock(*(_generator->mutex));


aten/src/TH/generic/THTensorRandom.cpp

  THArgCheck(THTensor_(isContiguous)(self), 1, "RNG state needs to be contiguous");
  rng_state = (THGenerator *)THTensor_(data)(self);
  THGenerator_copy(rng_state, _generator);
+  rng_state->mutex = NULL;  // mutex should not be part of the generator state


aten/src/TH/vector/avx_mathfun.h

 _PS256_CONST_TYPE(inv_mant_mask, int, ~0x7f800000);

-_PS256_CONST_TYPE(sign_mask, int, 0x80000000);
+_PS256_CONST_TYPE(sign_mask, int, (int)0x80000000);


aten/src/TH/generic/THTensorRandom.cpp

  THArgCheck(THTensor_(isContiguous)(self), 1, "RNG state needs to be contiguous");
  rng_state = (THGenerator *)THTensor_(data)(self);
  THGenerator_copy(rng_state, _generator);
+  rng_state->mutex = NULL;  // mutex should not be part of the generator state


colesbury

Looks pretty good. Three comments inline.

aten/src/TH/vector/avx_mathfun.h

 _PS256_CONST_TYPE(inv_mant_mask, int, ~0x7f800000);

-_PS256_CONST_TYPE(sign_mask, int, 0x80000000);
+_PS256_CONST_TYPE(sign_mask, int, (int)0x80000000);


aten/src/TH/THRandom.cpp

 {
-    memcpy(self, from, sizeof(THGenerator));
+    THGeneratorState_copy(&self->gen_state, &from->gen_state);
+    new (&self->mutex) std::mutex();


aten/src/TH/generic/THTensorRandom.cpp

 {
-  static const size_t size = sizeof(THGenerator);
-  THGenerator *rng_state;
+  static const size_t size = sizeof(THGeneratorState);


colesbury

LGTM!

This reverts commit 96239dd.

* Revert "Clarify grad_input_mask documentation in derivatives.yaml (#4963)" This reverts commit 6f3266b. * Revert "fix triu and tril for zero-strided inputs on gpu (#4962)" This reverts commit 6c197c2. * Revert "Add mutex for CPU RNG and move TH to C++ (#4041)" This reverts commit 96239dd. * Revert "Support multivariate TransformedDistributions (#4937)" This reverts commit ca5071d. * Revert "Only check that arguments are Variables in VariableType (#4943)" This reverts commit d444379. * Revert "torch.set_num_threads sets MKL option too (#4949)" This reverts commit 2aaeec0.

alsrgv · 2018-02-01T00:18:09Z

I think this breaks C FFI plugins since those are built with C, rather than C++. Specifically, I get this error:

packages/torch/utils/ffi/../../lib/include/TH/THRandom.h:6:17: fatal error: mutex: No such file or directory
     #include <mutex>
                     ^
    compilation terminated.
    error: command 'gcc' failed with exit status 1

alsrgv · 2018-02-01T03:32:38Z

CC @yf225 @colesbury @soumith ^^

ezyang · 2018-02-01T03:58:51Z

Drat! I guess, if we want to support FFI, we're going to need some C extern blocks and a C-compatible header for FFI plugins to import T_T

alsrgv · 2018-02-01T04:11:36Z

Yeah, I actually wanted to write my header file in C++, but parser didn't support extern "C" and/or namespace constructs.

apaszke · 2018-02-01T10:55:04Z

There’s a new C++-based extensions system underway. It might be simpler to recommend moving to that, but we should figure out an easy way to migrate (since this is breaking)

alsrgv · 2018-02-01T19:04:48Z

@apaszke, I'd love to try new C++ extension out when it's ready. That said, would you support the C way for a while for backward compatibility?

soumith · 2018-02-01T20:03:08Z

yes we will support the C extension. We'll issue fixes shortly on this.

soumith · 2018-02-02T17:45:52Z

@alsrgv now fixed in master

alsrgv · 2018-02-02T19:10:51Z

@soumith, thanks for the quick turnaround!

soumith requested changes Dec 18, 2017

View reviewed changes

Stonesjtu mentioned this pull request Jan 23, 2018

Add mutex lock for CPU RNG #4798

Closed

yf225 force-pushed the rng_mutex branch 2 times, most recently from 285e369 to c76ce0d Compare January 24, 2018 01:24

Stonesjtu reviewed Jan 24, 2018

View reviewed changes

aten/src/TH/THRandom.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

apaszke reviewed Jan 24, 2018

View reviewed changes

aten/src/TH/THRandom.cpp Outdated

This comment was marked as off-topic.

Sign in to view

yf225 force-pushed the rng_mutex branch from c76ce0d to bb09959 Compare January 24, 2018 21:19

yf225 added 3 commits January 26, 2018 00:17

Add mutex for CPU RNG

d86f817

move more things to cpp to make cuda build work

e8c82ec

fix mutex bug on OS X

ee143ff

yf225 force-pushed the rng_mutex branch from 363efe4 to ee143ff Compare January 26, 2018 05:17

try to fix cuda9 half .x bug

68720a6

yf225 force-pushed the rng_mutex branch from 8bedc2b to 68720a6 Compare January 26, 2018 07:30

try to fix windows error

2b648a6

yf225 changed the title ~~Add mutex for CPU RNG~~ Add mutex for CPU RNG and move TH to C++ Jan 26, 2018

apaszke suggested changes Jan 29, 2018

View reviewed changes

colesbury reviewed Jan 29, 2018

View reviewed changes

create THGeneratorState as seperate field

9afb34d

colesbury reviewed Jan 30, 2018

View reviewed changes

fix mutex issues

4028540

colesbury approved these changes Jan 31, 2018

View reviewed changes

soumith merged commit 96239dd into pytorch:master Jan 31, 2018

yf225 mentioned this pull request Jan 31, 2018

make CPU RNG thread-safe #3794

Closed

ssnl added a commit to ssnl/pytorch that referenced this pull request Jan 31, 2018

Revert "Add mutex for CPU RNG and move TH to C++ (pytorch#4041)"

cac3025

This reverts commit 96239dd.

yf225 mentioned this pull request Feb 2, 2018

Fix C FFI extension after moving TH to C++ #5005

Merged

soumith added the 0.3.1 label Feb 4, 2018

neerajprad mentioned this pull request Feb 5, 2018

Unable to build from source #5038

Closed

soumith removed the 0.3.1 label Feb 11, 2018

ezyang mentioned this pull request Apr 5, 2018

THC_createRandomSeed is not thread-safe #6288

Closed

ezyang mentioned this pull request Apr 23, 2019

[CPU] Refactor Random Number Generators in ATen - Part 1/n #16604

Closed

Add mutex for CPU RNG and move TH to C++ #4041

Add mutex for CPU RNG and move TH to C++ #4041

Uh oh!

Conversation

yf225 commented Dec 5, 2017

Uh oh!

colesbury commented Dec 5, 2017

Uh oh!

colesbury commented Dec 5, 2017

Uh oh!

soumith left a comment

Choose a reason for hiding this comment

Uh oh!

soumith commented Jan 23, 2018

Uh oh!

yf225 commented Jan 23, 2018

Uh oh!

soumith commented Jan 23, 2018

Uh oh!

apaszke commented Jan 23, 2018

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

yf225 commented Jan 24, 2018

Uh oh!

ezyang commented Jan 24, 2018

Uh oh!

This comment was marked as off-topic.

Uh oh!

colesbury commented Jan 24, 2018

Uh oh!

colesbury commented Jan 24, 2018

Uh oh!

yf225 commented Jan 24, 2018

Uh oh!

yf225 commented Jan 24, 2018

Uh oh!

colesbury commented Jan 24, 2018

Uh oh!

colesbury commented Jan 24, 2018

Uh oh!

yf225 commented Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yf225 commented Jan 26, 2018

Uh oh!

yf225 commented Jan 29, 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!

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!

This comment was marked as off-topic.

Uh oh!

yf225 commented Jan 25, 2018 •

edited

Loading