Summary: ReceivedUdpPacket has attached metadata (currently timings and later in the stack, tos values). Rather than passing the metadata separately in the read path for the client and the server, this propagates the ReceivedUdpPacket further up so this metadata is easier to use in the rest of the stack.
Reviewed By: mjoras
Differential Revision: D54912162
fbshipit-source-id: c980d5b276704f5bba780ac16a380bbb4e5bedad
Summary: I'm putting this up to get some initial feedback. I'm writing a function to get the peer transport parameters so that we can get them in the HQSession and set the QUIC fingerprint.
Reviewed By: hanidamlaj
Differential Revision: D54399572
fbshipit-source-id: 594294c26a5b34bc99a8d286a66be9cdbe7fff02
Summary: Adding virtual method getSelfCertificate to QuicSocket that's overriden in QuicServerTransport that returns the server cert.
Reviewed By: mjoras
Differential Revision: D54234662
fbshipit-source-id: fcef6399bc660ff41243336aacdfadd6e8975193
Summary: As title. Valid knob value range for KEY_UPDATE_INTERVAL is [1K, 8M] packets.
Reviewed By: mjoras
Differential Revision: D53335118
fbshipit-source-id: 2741763c34c07eb98c820c794c9aafe2625c00de
Summary:
Allow the server/client transport to initiate periodic key update. It's defaulted to being disabled.
The new logic for initiating and verifying a key update was handled correctly by the peer is consolidated in QuicTransportFunctions.
Reviewed By: mjoras
Differential Revision: D53109624
fbshipit-source-id: 0c3a944978fc0e0a84252da953dc116aa7c26379
Summary:
The current open connection counting is broken. To fix it this change:
- moves the counter to be for both the server and client transport
- calls onNewConnection() when the transport is ready.
- calls onConnectionClose() when the transport is closed after it was ready.
Reviewed By: lnicco, frankfeir
Differential Revision: D52638628
fbshipit-source-id: bcff7a8c671f8eede53c22e698e1b95332c56959
Summary:
This is the major transition that updates mvfst code to use the new interfaces. The new Folly implementations of the interfaces maintain all the existing behavior of folly types so this should not introduce any functional change. The core changes are:
- Update the BatchWriters to use the new interfaces.
- Update the FunctionLooper to use the new interfaces.
- Change QuicServerTransport to take the folly types and wrap them in the new types for use in the QuicTransportBase.
The rest of the diff is for updating all the existing uses of the QuicTrasnport to initialize the necessary types and pass them to the QUIC transport instead of directly passing folly types.
Reviewed By: mjoras
Differential Revision: D51413481
fbshipit-source-id: 5ed607e12b9a52b96148ad9b4f8f43899655d936
Summary: There is no reason to let the TLS layer control this.
Reviewed By: jbeshay
Differential Revision: D51267586
fbshipit-source-id: 6f70b7f6ba51a9a022195f7e2c1683b5323fde7a
Summary:
This diff renames `ReceivedPacket` to `ReceivedUdpPacket` to clarify that it maps to a UDP packet and not a QUIC packet. A single UDP packet can contain multiple QUIC packets due to coalescing.
--
This diff is part of a larger stack focused on the following:
- **Cleaning up client and server UDP packet receive paths while improving testability.** We currently have multiple receive paths for client and server. Capabilities vary significantly and there are few tests. For instance:
- The server receive path supports socket RX timestamps, abet incorrectly in that it does not store timestamp per packet. In comparison, the client receive path does not currently support socket RX timestamps, although the code in `QuicClientTransport::recvmsg` and `QuicClientTransport::recvmmsg` makes reference to socket RX timestamps, making it confusing to understand the capabilities available when tracing through the code. This complicates the tests in `QuicTypedTransportTests`, as we have to disable test logic that depends on socket RX timestamps for client tests.
- The client currently has three receive paths, and none of them are well tested.
- **Modularize and abstract components in the receive path.** This will make it easier to mock/fake the UDP socket and network layers.
- `QuicClientTransport` and `QuicServerTransport` currently contain UDP socket handling logic that operates over lower layer primitives such `cmsg` and `io_vec` (see `QuicClientTransport::recvmmsg` and `...::recvmsg` as examples).
- Because this UDP socket handling logic is inside of the mvfst transport implementations, it is difficult to test this logic in isolation and mock/fake the underlying socket and network layers. For instance, injecting a user space network emulator that operates at the socket layer would require faking `folly::AsyncUDPSocket`, which is non-trivial given that `AsyncUDPSocket` does not abstract away intricacies arising from the aforementioned lower layer primitives.
- By shifting this logic into an intermediate layer between the transport and the underlying UDP socket, it will be easier to mock out the UDP socket layer when testing functionality at higher layers, and inject fake components when we want to emulate the network between a mvfst client and server. It will also be easier for us to have unit tests focused on testing interactions between the UDP socket implementation and this intermediate layer.
- **Improving receive path timestamping.** We only record a single timestamp per `NetworkData` at the moment, but (1) it is possible for a `NetworkData` to have multiple packets, each with their own timestamps, and (2) we should be able to record both userspace and socket timestamps.
Reviewed By: silver23arrow
Differential Revision: D48788809
fbshipit-source-id: 3793c30212d545e226f3e5337289bc2601dfa553
Summary:
When the server accepts a 0-rtt ticket, it updates the connection's transport settings with the contents of the ticket. This is the value used included in the next ticket it sends the client. However, the handshake layer has a copy of the original transport parameters that was created with the first received packet. This copy in the handshake layer does not get updated. This can cause a mismatch between the value sent to the client in the handshake, and the value encoded inside the ticket.
This change avoid using the 0-rtt ticket for updating any transport settings that are also included in the handshake transport params.
Reviewed By: hanidamlaj
Differential Revision: D51121317
fbshipit-source-id: 55e71965185dff553d16d4c5fbcb1e2f9acdc690
Summary:
This diff drops `NetworkDataSingle` in favor of `ReceivedPacket`. The latter contains a `ReceivedPacket::Timings` field that has the same `receiveTimePoint` currently in `NetworkDataSingle`, while also providing other useful signals.
--
This diff is part of a larger stack focused on the following:
- **Cleaning up client and server UDP packet receive paths while improving testability.** We currently have multiple receive paths for client and server. Capabilities vary significantly and there are few tests. For instance:
- The server receive path supports socket RX timestamps, abet incorrectly in that it does not store timestamp per packet. In comparison, the client receive path does not currently support socket RX timestamps, although the code in `QuicClientTransport::recvmsg` and `QuicClientTransport::recvmmsg` makes reference to socket RX timestamps, making it confusing to understand the capabilities available when tracing through the code. This complicates the tests in `QuicTypedTransportTests`, as we have to disable test logic that depends on socket RX timestamps for client tests.
- The client currently has three receive paths, and none of them are well tested.
- **Modularize and abstract components in the receive path.** This will make it easier to mock/fake the UDP socket and network layers.
- `QuicClientTransport` and `QuicServerTransport` currently contain UDP socket handling logic that operates over lower layer primitives such `cmsg` and `io_vec` (see `QuicClientTransport::recvmmsg` and `...::recvmsg` as examples).
- Because this UDP socket handling logic is inside of the mvfst transport implementations, it is difficult to test this logic in isolation and mock/fake the underlying socket and network layers. For instance, injecting a user space network emulator that operates at the socket layer would require faking `folly::AsyncUDPSocket`, which is non-trivial given that `AsyncUDPSocket` does not abstract away intricacies arising from the aforementioned lower layer primitives.
- By shifting this logic into an intermediate layer between the transport and the underlying UDP socket, it will be easier to mock out the UDP socket layer when testing functionality at higher layers, and inject fake components when we want to emulate the network between a mvfst client and server. It will also be easier for us to have unit tests focused on testing interactions between the UDP socket implementation and this intermediate layer.
- **Improving receive path timestamping.** We only record a single timestamp per `NetworkData` at the moment, but (1) it is possible for a `NetworkData` to have multiple packets, each with their own timestamps, and (2) we should be able to record both userspace and socket timestamps
Reviewed By: silver23arrow
Differential Revision: D48739219
fbshipit-source-id: fc2cdb7b425d68c729dd3bec00b6c6ff3c4bf8ec
Summary:
This diff:
1. Introduces a new `ReceivedPacket::Timings` structure, which will be expanded upon in subsequent diffs.
2. Adds a `ReceivedPacket::Timings` field to each `ReceivedPacket`
3. Uses the accessors added in the previous diff (D48724715) to populate the `ReceivedPacket::Timings` structure in each `ReceivedPacket` held by a `NetworkData` object. This is done by propagating the `NetworkData::receiveTimePoint` field to all `ReceivedPacket` held in a `NetworkData.` This propagation occurs each time a `ReceivedPacket` is added. The value is propagated again if the `NetworkData::receiveTimePoint` field is updated by looping over all previously added `ReceivedPacket`.
--
This diff is part of a larger stack focused on the following:
- **Cleaning up client and server UDP packet receive paths while improving testability.** We currently have multiple receive paths for client and server. Capabilities vary significantly and there are few tests. For instance:
- The server receive path supports socket RX timestamps, abet incorrectly in that it does not store timestamp per packet. In comparison, the client receive path does not currently support socket RX timestamps, although the code in `QuicClientTransport::recvmsg` and `QuicClientTransport::recvmmsg` makes reference to socket RX timestamps, making it confusing to understand the capabilities available when tracing through the code. This complicates the tests in `QuicTypedTransportTests`, as we have to disable test logic that depends on socket RX timestamps for client tests.
- The client currently has three receive paths, and none of them are well tested.
- **Modularize and abstract components in the receive path.** This will make it easier to mock/fake the UDP socket and network layers.
- `QuicClientTransport` and `QuicServerTransport` currently contain UDP socket handling logic that operates over lower layer primitives such `cmsg` and `io_vec` (see `QuicClientTransport::recvmmsg` and `...::recvmsg` as examples).
- Because this UDP socket handling logic is inside of the mvfst transport implementations, it is difficult to test this logic in isolation and mock/fake the underlying socket and network layers. For instance, injecting a user space network emulator that operates at the socket layer would require faking `folly::AsyncUDPSocket`, which is non-trivial given that `AsyncUDPSocket` does not abstract away intricacies arising from the aforementioned lower layer primitives.
- By shifting this logic into an intermediate layer between the transport and the underlying UDP socket, it will be easier to mock out the UDP socket layer when testing functionality at higher layers, and inject fake components when we want to emulate the network between a mvfst client and server. It will also be easier for us to have unit tests focused on testing interactions between the UDP socket implementation and this intermediate layer.
- **Improving receive path timestamping.** We only record a single timestamp per `NetworkData` at the moment, but (1) it is possible for a `NetworkData` to have multiple packets, each with their own timestamps, and (2) we should be able to record both userspace and socket timestamps.
Reviewed By: jbeshay
Differential Revision: D48725209
fbshipit-source-id: 580e7d7d1f3587f9947774b5ed19e9985df404c9
Summary:
`connSetupCallback_` and `connCallback_` in `QuicTransportBase.h` are raw pointers, which delegates the responsibility to keep these callbacks alive to the caller.
There are use cases where it would be convenient to be able to tie the lifetime of the callback to the Quic transport, e.g,. as long as the Quic transport is alive, it keeps the callbacks alive as well.
This diff uses MaybeManagedPtr to achieve this lifetime tie if desired. A MaybeManagedPtr intialized with a shared pointer manages lifetime of the contained object, whereas a MaybeManagedPtr initialized with a raw pointer does not manage lifetime of the contained object. This way caller can decide to pass in a shared ptr or raw pointer and achieve the desired behavior.
Note that we cannot simply use a shared_ptr for that. Using a shared_ptr would potentially mean that callbacks passed are destroyed when the transport is destroyed. Callbacks would not be destroyed if they were managed by a shared_ptr already, but this is something we cannot assume for every case. This would thus be a change in semantics to the current implementation, where the callbacks can outlive the transport.
Reviewed By: mjoras
Differential Revision: D49502381
fbshipit-source-id: 771a9328b99dc4f94f8e9679f9caf98af9180428
Summary: This enables the server to include a cwnd hint in the 0-rtt ticket it sends to the client.
Reviewed By: mjoras
Differential Revision: D43131826
fbshipit-source-id: 742e4e531027ec6618a1b761c450b507368e5a2f
Summary:
This diff changes `QuicAsyncUDPSocketWrapper` so that it is an abstraction layer that inherits from `QuicAsyncUDPSocketType`, instead of simply being a container with aliases.
- Key changes in `QuicAsyncUDPSocketWrapper.h`, the rest of the updates switch us from using `QuicAsyncUDPSocketType` to `QuicAsyncUDPSocketWrapper`.
- It's difficult to mock the UDP socket today given that we expose the entire `folly::AsyncUDPSocket` type to the higher layers of the QUIC stack. This complicates testing and emulation because any mock / fake has to implement low level primitives like `recvmmsg`, and because the `folly::AsyncUDPSocket` interface can change over time.
- Pure virtual functions will be defined in `QuicAsyncUDPSocketWrapper` in a follow up diff to start creating an interface between the higher layers of the mvfst QUIC stack and the UDP socket, and this interface will abstract away lower layer details such as `cmsgs` and `io_vec`, and instead focus on populating higher layer structures such as `NetworkData` and `ReceivedPacket` (D48714615). This will make it easier for us to mock or fake the UDP socket.
This diff relies on changes to `folly::MockAsyncUDPSocket` introduced in D48717389.
--
This diff is part of a larger stack focused on the following:
- **Cleaning up client and server UDP packet receive paths while improving testability.** We currently have multiple receive paths for client and server. Capabilities vary significantly and there are few tests. For instance:
- The server receive path supports socket RX timestamps, abet incorrectly in that it does not store timestamp per packet. In comparison, the client receive path does not currently support socket RX timestamps, although the code in `QuicClientTransport::recvmsg` and `QuicClientTransport::recvmmsg` makes reference to socket RX timestamps, making it confusing to understand the capabilities available when tracing through the code. This complicates the tests in `QuicTypedTransportTests`, as we have to disable test logic that depends on socket RX timestamps for client tests.
- The client currently has three receive paths, and none of them are well tested.
- **Modularize and abstract components in the receive path.** This will make it easier to mock/fake the UDP socket and network layers.
- `QuicClientTransport` and `QuicServerTransport` currently contain UDP socket handling logic that operates over lower layer primitives such `cmsg` and `io_vec` (see `QuicClientTransport::recvmmsg` and `...::recvmsg` as examples).
- Because this UDP socket handling logic is inside of the mvfst transport implementations, it is difficult to test this logic in isolation and mock/fake the underlying socket and network layers. For instance, injecting a user space network emulator that operates at the socket layer would require faking `folly::AsyncUDPSocket`, which is non-trivial given that `AsyncUDPSocket` does not abstract away intricacies arising from the aforementioned lower layer primitives.
- By shifting this logic into an intermediate layer between the transport and the underlying UDP socket, it will be easier to mock out the UDP socket layer when testing functionality at higher layers, and inject fake components when we want to emulate the network between a mvfst client and server. It will also be easier for us to have unit tests focused on testing interactions between the UDP socket implementation and this intermediate layer.
- **Improving receive path timestamping.** We only record a single timestamp per `NetworkData` at the moment, but (1) it is possible for a `NetworkData` to have multiple packets, each with their own timestamps, and (2) we should be able to record both userspace and socket timestamps.
Reviewed By: jbeshay, hanidamlaj
Differential Revision: D48717388
fbshipit-source-id: 4f34182a69ab1e619e454da19e357a6a2ee2b9ab
Summary:
This diff:
- Changes `NetworkDataSingle` to have `ReceivedPacket` instead of `Buf`, in line with earlier change to `NetworkData` in D48714615
--
This diff is part of a larger stack focused on the following:
- **Cleaning up client and server UDP packet receive paths while improving testability.** We currently have multiple receive paths for client and server. Capabilities vary significantly and there are few tests. For instance:
- The server receive path supports socket RX timestamps, abet incorrectly in that it does not store timestamp per packet. In comparison, the client receive path does not currently support socket RX timestamps, although the code in `QuicClientTransport::recvmsg` and `QuicClientTransport::recvmmsg` makes reference to socket RX timestamps, making it confusing to understand the capabilities available when tracing through the code. This complicates the tests in `QuicTypedTransportTests`, as we have to disable test logic that depends on socket RX timestamps for client tests.
- The client currently has three receive paths, and none of them are well tested.
- **Modularize and abstract components in the receive path.** This will make it easier to mock/fake the UDP socket and network layers.
- `QuicClientTransport` and `QuicServerTransport` currently contain UDP socket handling logic that operates over lower layer primitives such `cmsg` and `io_vec` (see `QuicClientTransport::recvmmsg` and `...::recvmsg` as examples).
- Because this UDP socket handling logic is inside of the mvfst transport implementations, it is difficult to test this logic in isolation and mock/fake the underlying socket and network layers. For instance, injecting a user space network emulator that operates at the socket layer would require faking `folly::AsyncUDPSocket`, which is non-trivial given that `AsyncUDPSocket` does not abstract away intricacies arising from the aforementioned lower layer primitives.
- By shifting this logic into an intermediate layer between the transport and the underlying UDP socket, it will be easier to mock out the UDP socket layer when testing functionality at higher layers, and inject fake components when we want to emulate the network between a mvfst client and server. It will also be easier for us to have unit tests focused on testing interactions between the UDP socket implementation and this intermediate layer.
- **Improving receive path timestamping.** We only record a single timestamp per `NetworkData` at the moment, but (1) it is possible for a `NetworkData` to have multiple packets, each with their own timestamps, and (2) we should be able to record both userspace and socket timestamps.
Reviewed By: mjoras
Differential Revision: D48714796
fbshipit-source-id: d96c2abc81e7c27a01bcd0dd552f274a0c1ede26
Summary: Update flow control settings names to reflect that these are indeed flow control
Reviewed By: jbeshay
Differential Revision: D48137685
fbshipit-source-id: a48372e21cdd529480e25785a9bd5de456427ef3
Summary: Ensure all CCA-related settings are included in setCongestionControl(), and that it's called from the knob setting the congestion controller.
Reviewed By: kvtsoy
Differential Revision: D47741830
fbshipit-source-id: ff1d2347581c61a58f2caaff8189126930bf4e04
Summary: Rename transportSetting.BBRConfig to CongestionControlConfig to prepare it for being used by other CCAs too.
Reviewed By: hanidamlaj, mjoras
Differential Revision: D47309439
fbshipit-source-id: 56d0ddc9752789709c54a4f7cc595f94c656e49e
Summary:
The pacingTimerTickInterval transport setting conflates two options: the minimum interval a pacer can use, and the resolution of the underlying timer. This means that a higher value leads to lower timer resolution and less pacing accuracy.
This change adds a separate parameter for the timer resolution to ensure that a larger pacing tick does not degrade the pacer accuracy.
Reviewed By: mjoras
Differential Revision: D46564066
fbshipit-source-id: 0d0e54077b80da03e6e6c9baaab49a4c969966b6
Summary: We don't use it, and the OSS lib hasn't been updated in a while.
Reviewed By: mjoras
Differential Revision: D46707559
fbshipit-source-id: ec102a52183a736cfb1c0241600816a837062108
Summary:
Create and use an actual wrapper around folly::EventBase.
Later an interface will be added that the wrapper will be implementing.
Reviewed By: jbeshay
Differential Revision: D45822401
fbshipit-source-id: 3b33f796c31043ec2881b753a9b60943bdf91f1d
Summary:
Right now small packets will cause us to subtract from the packet limit during write loops. This is generally okay when using just non-DSR data. For DSR though there is an issue where if we only write a single small packet from the non-DSR path (like an ACK) we will discount it from the amount of packets the DSR path can write.
The existing workaround for this is to only discount the non-DSR path write if the number of bytes written is less than half a packet's worth. This heuristic is incomplete though and doesn't consider the cases like the following:
1. Packet limit is 5
2. Normal path writes 4 packets of size 1232, 1232, 1232, 20.
3. The DSR path only gets one packet.
The reason for this is because the normal path also wrote full packets, but ended with a short packet. The account for this we just have to discount in terms of full packet's written. In the above example, the DSR path would get the chance to write two packets. Thus we would exceed the packet limit by one packet, but it would get us closer to the desired amount of data on the wire.
Reviewed By: kvtsoy
Differential Revision: D46633105
fbshipit-source-id: 00d92499ab73fc746ea5fdb6ff31e10f06b98666
Summary:
The idea here is to add a notion of time-based sampling of certain QUIC_STATS. This allows accounting to be done via consistent distributions for comparisons.
For now limit to the server, and only implement for inflight bytes, SRTT, and CCA bandwidth.
Reviewed By: jbeshay
Differential Revision: D46410903
fbshipit-source-id: a5db1ec720a0f8bf54e04d66c0d68686660e8eaa
Summary:
The idea here is to allow a way to do incremental stream priorities while switching between streams as aggressively.
Achieving this is somewhat tricky. The easiest place to track this is to make it so the iterator in QuicPriorityQueue no-op next() until a counter reaches an increment.
This is especially impacftul for DSR, where round robining per packet is almost pathologically bad both for CPU impact but also spurious losses and low bandwidth estimates.
Thoughts?
(Note: this ignores all push blocking failures!)
Reviewed By: kvtsoy
Differential Revision: D46268308
fbshipit-source-id: cd5b924141365f61f8a3363bc9cb38a62e5c94cf
Summary:
The write loop functions for DSR or non-DSR are segmented today. As such, so are the schedulers. Mirroring this, we also currently store the DSR and non-DSR streams in separate write queues. This makes it impossible to effectively balance between the two without potential priority inversions or starvation.
Combining them into a single queue eliminates this possibility, but is not entirely straightforward.
The main difficulty comes from the schedulers. The `StreamFrameScheduler` for non-DSR data essentially loops over the control stream queue and the normal write queue looking for the next stream to write to a given packet. When the queues are segmented things are nice and easy. When they are combined, we have to deal with the potential that the non-DSR scheduler will hit a stream with only DSR data. Simply bailing isn't quite correct, since it will just cause an empty write loop. To fix that we need check, after we are finished writing a packet, if the next scheduled stream only has DSR data. If it does, we need to ensure `hasPendingData()` returns false.
The same needs to be done in reverse for the DSR stream scheduler.
The last major compication is that we need another loop which wraps the two individual write loop functions, and calls both functions until the packet limit is exhausted or there's no more data to write. This is to handle the case where there are, for example, two active streams with the same incremental priority, and one is DSR and the other is not. In this case each write loop we want to write `packetLimit` packets, flip flopping between DSR and non DSR packets.
This kind of round robining is pathologically bad for DSR, and a future diff will experiment with changing the round robin behavior such that we write a minimum number of packets per stream before moving on to the next stream.
This change also contains some other refactors, such as eliminating `updateLossStreams` from the stream manager.
(Note: this ignores all push blocking failures!)
Reviewed By: kvtsoy
Differential Revision: D46249067
fbshipit-source-id: 56a37c02fef51908c1336266ed40ac6d99bd14d4
Summary: We shouldn't attempt to write DSR data if we run out of congestion control bytes. This will just cause an empty DSR write, which shouldn't have any negative impact but it causes tperf spam.
Reviewed By: jbeshay
Differential Revision: D46126190
fbshipit-source-id: 390e04c5d6182a2e0e61d63e806522d1ea410a30
Summary:
This has been hardcoded to SRTT/25 for a long time. However, especially when using DSR this might not be the most appropriate since it can start to get close to real world SRTTs.
Control it via a knob, and also add the behavior such that setting it to 0 effectively disables the time limit.
Reviewed By: jbeshay
Differential Revision: D46084438
fbshipit-source-id: 7dc619fdff1bd5c3f8654c4936200e0340ef94f2
Summary: Allows the default stream priority (which also means scheduling policy) to be set via TransportSettings.
Reviewed By: jbeshay, hanidamlaj
Differential Revision: D45881729
fbshipit-source-id: fcb72022cd6eac2f1dafc55173d5a46a72a95dbc
Summary: The log for PACING_TIMER_TICK knob was referring to another knob name. This fixes it.
Reviewed By: sharmafb
Differential Revision: D45834217
fbshipit-source-id: c26c481633218dc165a65efcf029e7c636947dea
Summary: For non-DSR streams we can fill a packet that has e.g. stream limit updates and flow control with stream data. With DSR, we can't do that and thus packets that contain this data end up counting against the packet limit. This means that we often end up writing fewer packets per write loop when using DSR. Account for this by basically discounting the non-DSR packet in a write loop if it was "small".
Reviewed By: shodoco
Differential Revision: D45754930
fbshipit-source-id: ca494a4bd042de05c12a4d45e80dd5582fe12248
Summary:
Due to the fact that store DSR and non DSR streams in separate priority queues, we end up having to make a decision on which to write first.
Longer term we should merge them into a single priority queue, but in the meantime to mitigate potential starvation, randomly flip flop between which we write first in a given write loop.
Reviewed By: jbeshay, shodoco
Differential Revision: D45701230
fbshipit-source-id: 894179e457a4d6f7364767108f9290ff267eb977
