[RFC] Added unpaddedSize_ to Tensor

[gcatron]

Summary:
This PR adds a new field to Tensor: unpaddedSize_.
Since we have a static batch size we copy the entire tensor to the device regardless of number of inputs. This would allow a DeviceManager to only copy the inputs if it supports it.
This means adding additional metaData to Tensor, this seemed cleaner than passing side channel data.

Documentation: NA

Test Plan: ninja test

[bertmaher]

I had a pretty evil thought: do we ever check that the Tensor and bound Placeholder in PlaceholderBindings actually have the same type? If we relax that restriction, we could simply bind smaller tensors.

I'm not sure that's a good idea, I'm just thinking about ways to avoid cluttering the main Tensor class.

[bertmaher]

Here's a different thought that feels safer than binding wrong-sized tensors... we already create an ExecutionContext that gets passed all the way down to the execute method where we need the size; maybe we should keep a parallel map of physical sizes in that object.

[bertmaher]

If we go with the design in this PR (passing the unpadded size in the Tensor) I'd like to make it a bit safer than simply adding a getter/setter. Some suggestions:

1. This optimization only makes sense for unowned tensors; add a constructor for this case:

Tensor(void *data, TypeRef ty, size_t unpaddedSize);

2. Only provide a getUnpaddedSize(), and have it return size() if the tensor is not padded. This is what you have.
3. I guess I'm nervous about setting unpaddedSize=0 to mean "the tensor is not padded". I'd prefer to always set unpaddedSize, but I guess this is a bummer because you have to do some multiplications every time you allocate a Tensor.
4. Maybe call the convenience method getUnpaddedSizeInBytes() to follow convention elsewhere.

[bertmaher]

If I'm right that virtually-padded tensors are only valid if unowned, assert that the tensor is unowned here.

[gcatron]

Here's a different thought that feels safer than binding wrong-sized tensors... we already create an ExecutionContext that gets passed all the way down to the execute method where we need the size; maybe we should keep a parallel map of physical sizes in that object.

Hmm, maybe we add it to PlaceholderBindings, I'm not sure I like that better than metaData directly on the Tensor but that would work too... I guess the question there is, Do we ever create a padded Tensor so far upstream that it'd be a pain to plumb the padding factor down to context creation?

[bertmaher]

OK, after sleeping on it I'm on board with the add-it-to-Tensor approach :-)

[bertmaher]

What do you think about killing the setter in favor of a new flavor of unowned constructor? I always like to avoid exposing data members that can be arbitrarily twiddled.

[bertmaher]

I'm comfortable approving this, esp to unblock work on top of it.

[jfix71]

Would it make sense to add this to Type instead of Tensor?

[nadavrot]

CC @opti-mix who is looking into generalizing the padding support in the type system.

[opti-mix]

Would it make sense to add this to Type instead of Tensor?

I've implemented some support for providing custom strides/alignments for dimensions of tensors on my private branch ( related to #2686 ). E.g. you can say that a stride for a given dimension is a multiple of 64. If you only have 3 elements as the actual size of the dimension, the remaining 61 elements would be just "padding" elements filled with some garbage data.

I've been planning to upstream it, though not necessarily in the coming weeks. On this branch, I've added a new strides_ member to Type and updated related places affected by this change.

If you think that the ability to specify strides on a per-dimension basis would help with your issue, we could upstream it earlier than I initially planned.

But I'm under the impression that you are trying to do something different here.

[bertmaher]

To provide context here, the problem we're trying to solve is that PCIe traffic sending inputs to devices is a bottleneck when we're using SparseLengthsSum operators. The problem is that we're forced to provide a maximally-sized "indices" input, when it fact we often use fewer than the maximum number of indices. (E.g., we might compile to a sequence length of 1000, but frequently use only 100). So we end up transferring a bunch of zeros (or garbage) over PCIe, despite knowing (at runtime) the correct amount of data to transfer.

I think this feature is probably orthogonal to strided tensors; it's more like cheapo support for dynamic shapes ;-).

[bertmaher]

Would it make sense to add this to Type instead of Tensor?

That could be reasonable... although I'm not sure whether it's really a type-system property. I've been thinking of it more as a storage property of the Tensor, like the type is still Int64[2000] but it's "virtually padded" as needed rather than being physically padded to the right size.

[opti-mix]

I'd suggest not merging this PR yet. I've got some ideas I'd like to discuss next week.

[nickgg]

Initially I preferred the Tensor approach, but I'm leaning towards adding it to Type now - what does it mean to e.g. MatMul two tensors with the same Type but different paddings?

[bertmaher]

You could actually use this for matmul to express padding in the batch dimension. There are maybe two concepts here, (1) can this dimension be padded, which could go in Type, and (2) how much padding is there actually, which probably has to go on Tensor (or some other runtime structure).

[bertmaher]

lgtm, thanks!

[facebook-github-bot]

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

[facebook-github-bot]

@gcatron merged this pull request in ee5e2fd.