tf-coriander icon indicating copy to clipboard operation
tf-coriander copied to clipboard

Ensure InlinedVector works on Mac, for `InUse` structs

Open hughperkins opened this issue 7 years ago • 1 comments

InlinedVector doesnt work on Mac, for InUse structs

  • means the eventmgr doesnt work on Mac, unless changed to use a normal std::vector (which was done in https://github.com/hughperkins/tf-coriander/compare/8a02ae2a3a731...e97b99408178d to fix https://github.com/hughperkins/tf-coriander/issues/34 )
  • however, would be nice to fix the inlinedvector for Mac

There is an opportunity for someone to look into why the InlinedVector doesnt work for InUse objects, and specificlaly for objects containing std::function, I think, so we can switch back to InlinedVecvtor.

To start work on this issue:

  • download latest tf-coriander
  • in tensorflow/core/common_runtime/gpu/gpu_event_mgr.h, look for the line typedef std::vector<InUse> ToFreeVector;
    • change this line to be typedef gtl::InlinedVector<InUse, 4> ToFreeVector;
  • also change the signature of PollEvents, in tensorflow/core/common_runtime/gpu/gpu_event_mgr.cc, to reflect this change
  • ./configure, and do a build
  • try running something
  • segfault :-(

Benefits of fixing this:

  • maybe faster

Anything to check before working on this?

  • you could compare the performance with using InlinedVector vs std::vector, in the gpu_event_mgr, even in the presence of the segfault, and see if it actually changes anything
  • if the performance is the same, then you could report that here, and we'll close this out, leave it as std::vector

By the way, how to fix this?

  • I reckon it might be sufficient to add a std::function to the aligment union in tensorflow//core/lib/gtl/inlined_vector.h, for the u_ member
  • in other words, I think the issue is an alignment issue, since maybe std::function requires larger alignment than a single pointer?

hughperkins avatar May 30 '17 08:05 hughperkins

I have the same issue with clang 3.8 when using libc++ (which is probably why you only saw this on Mac). The workaround works on Linux too.

nicolov avatar Mar 02 '18 03:03 nicolov