🔗 Helpful links

• 🧪  See artifacts and rendered test results at hud.pytorch.org/pr/81824
• 📄  Preview Python docs built from this PR
• 📄  Preview C++ docs built from this PR
• ❓Need help or want to give feedback on the CI? Visit our office hours

✅ No Failures (0 Pending)

As of commit 44f09aa (more details on the Dr. CI page):

I was very surprised by how expensive it is to copy around SizesAndStrides. (I assume the copy from TensorImpl::sizes_and_strides() get's RVO'd through to the emplace so you only do one copy?) Benchmarking your PR on my system adds about 180 ns for one input. (An additional 40% to the profiling overhead.) That got me curious, and I played around with all manner of unsafe data access to the underlying SizesAndStrides and its underlying memory. Ultimately what I found was that just calling t.sizes() and t.strides() was as fast as any of the specializations (read: hacks) that I could come up with. That said, I did find that when I added a vectorized copy to AppendOnlyList there was a non-trivial speedup:

  template <typename T0>
  typename std::enable_if<
      std::is_same<T0, T>::value && std::is_trivially_copyable<T>::value>::type
  copy(const T0* src, size_t n) {
    if (C10_LIKELY(next_ + n <= end_)) {
      std::memcpy((void*)next_, (void*)src, n * sizeof(T0));
      next_ += n;
    } else {
      // We could chunk this into several `memcpy`s, but because we expect this
      // fallback to be infrequent (n << ChunkSize) the performance impact is
      // negligible.
      for (size_t i = 0; i < n; i++) {
        emplace_back(*src++);
      }
    }
  }

Interestingly, I also found that doing separate memcpy's for sizes and strides was faster than a single memcpy of the contiguous memory. (In addition to using less memory) When I did that (again, nothing fancy)

    const auto& sizes = t.sizes();
    const auto& strides = t.strides();
    const auto dim = sizes.size();
    ...
    tensor_sizes_.copy(sizes.data(), dim);
    tensor_sizes_.copy(strides.data(), dim);

overhead was identical to the baseline. So I think that (plus updating the decode) is the way to go. Sorry for leading you astray. WDYT?

Firstly, we should discuss what we want from TensorImpl ( search TensorImpl size constraints) .
Below is my best guess of what I think we want from it.

+ is we do want, ? is unsure, - is we don't want

- various other pointers
- Refcount information
+ storage pointer (temporarily to get Unique Tensor Id)
- SizesAndStrides (from discussion above, we will capture this in a different fashion)
? storage_offset
- numel (duplicated from Sizes information)
+ dtype 
+ device 
- is_contiguous (John confirmed that this is just a bit to optimize performance on TensorImpl)
- storage_access_should_throw_ 
? bitfields - Need to review on a bit by bit basis, many bits are just speed optimizations for TensorImpl.

I think from this, we can conclude that we do not want to copy TensorImpl, and likely want to do our own thing. We should align on what exactly we want from TensorImpl, and also check that there are no data structures that currently capture most of what we want.

Secondly, all of the multiple benchmarks, with different resulting times shows that it is critical that we have a repeatable way of running benchmarks, and that we migrate to a system that has reproducible results. It will be really hard to get anything done when one week we are saying that x feature has a 20% overhead, and the next, we say it has 40%. My proposal is that we do future benchmarking on the AWS machines that people are preparing specifically for benchmarking and move the benchmark script to open source, and fully document the benchmarking process for profiler overheads.

• Refcount information

I think we might want this. (CC @c-odrin who is looking into implementing it) The specific use case is to help find leaks by allowing the user to see where the number of referents differs from what is expected. An atomic read should be cheap, but this is all subject to cost-benefit analysis once we have something concrete.

? storage_offset

Probably not? I don't think we're expecting to get compiler-y enough to need this fine grained of detail.

? is_contiguous

I think duplicated from sizes and strides information?

? bitfields

Lots of great info for not much space is very tempting, but probably not until we have a concrete use case.

+1 we should open source the benchmark.

[Closing this as Taylor thinks we should not go with SizesAndStrides for this support and instead go back to what is implemented in #80072 ]