torch / tds

Torch C data structures
Other
80 stars 25 forks source link

tds.AtomicCounter #20

Closed lake4790k closed 8 years ago

lake4790k commented 8 years ago

Atomic counter that can safely track progress between threads. I'll add threads.sharedserialize support once this is merged in tds.

deltheil commented 8 years ago

The THAtomic.h header from torch is never included which gives implicit declaration of function ‘THAtomicAddLong’, etc. warnings. Since torch7 is not a hard dependency you should condition the availability of this atomic counter with the HAS_TORCH macro handled by cmake if torch is detected, e.g. see this.

lake4790k commented 8 years ago

Added the include with HAS_TORCH you suggested, also added basic mutex solution if no Torch is present (probably not that useful). This is expected to be used with Torch threads of course, so I didn't want to copy the whole THAtomic over.

deltheil commented 8 years ago

so I didn't want to copy the whole THAtomic over

I see. Another possibility to avoid reinventing the wheel (since anyway pthreads detection should also be included and all of this is already managed by torch7) could be to introduce a function like:

bool tds_has_atomic(void) {
#if HAS_TORCH
  return true;
#else
  return false;
#endif
}

(and of course bracket the implementation of the various functions with HAS_TORCH)

This could be used to avoid creating an atomic counter:

-- atomic.lua

function atomic:__new(...)
  if not C.tds_has_atomic() then
    error('atomic counter not available (Torch not found)')
  end
  -- ...
end
lake4790k commented 8 years ago

Thanks, implemented your suggestion, the implementation now is only available with Torch, because of the dependency on THAtomic, otherwise makes no sense to provide a useless implementation.

Should be good to go...?

deltheil commented 8 years ago

Cool! Looks good to me.

soumith commented 8 years ago

Thanks!