822129 - Reduce memcpys in MediaPipeline

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P4)

Description

[Eric Rescorla (:ekr)]

Every packet out requires a number of copies for thread
transitions and to accomodate SRTP overhead. See if we
can do something to reduce this.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: don't alloc/free on every packet send in MediaPipeline

Comment 2

[Eric Rescorla (:ekr)]

What does profiling show is the impact of this change?

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

This is tricky on a fast machine, and will vary according to allocator, memory pressure, fragmentation level, etc.  

Some comparative profiles show without this, malloc/free from SendRtpPacket_s() is around 0.5% of STS thread time.  With the patch, it never got hit at all by the profiler.  So it's a small win (maybe 0.1% overall, as STS thread is on the perf radar, but not the hottest thread), and might be larger on B2G/Android or perhaps on Windows.

It removes hundreds of allocations-per-second (50+ for audio, 30-200+ for video), per A/V stream.  And it's easy.

Comment 4

[Eric Rescorla (:ekr)]

Comment on attachment 739934 [details] [diff] [review]
don't alloc/free on every packet send in MediaPipeline

Review of attachment 739934 [details] [diff] [review]:
-----------------------------------------------------------------

MediaPipeline::SendRtpPacket() and MediaPipeline::SendRtcpPacket() both
already allocate DataBuffers to allow for an async thread dispatch.

nsresult MediaPipeline::PipelineTransport::SendRtpPacket(
    const void *data, int len) {

    nsAutoPtr<DataBuffer> buf(new DataBuffer(static_cast<const uint8_t *>(data),
                                             len));

    RUN_ON_THREAD(sts_thread_,
                  WrapRunnable(
                      RefPtr<MediaPipeline::PipelineTransport>(this),
                      &MediaPipeline::PipelineTransport::SendRtpPacket_s,
                      buf),
                  NS_DISPATCH_NORMAL);

    return NS_OK;
}

Instead of having a new member variable that we use to store the temp
buffer, we should just extend DataBuffer to have separate length
and size values (where size >= length). Then we can allocate an
oversized DataBuffer and pass it down, skipping all the memory
fussing around in SendRtpPacket_s().

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
@@ +157,5 @@
>  
> +    // SRTP needs larger buffers for Auth tags
> +    // Avoid reallocating SRTP packet buffer on every send
> +    int packet_size_;
> +    nsAutoPtr<unsigned char> packet_;

nsAutoPtr uses delete but because we allocated an array we need delete[]

This should be a ScopedDeleteArray, which does the right thing and also has stronger ownership semantics.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Attachment: don't alloc/free on every packet send in MediaPipeline

updated to expand the DataBuffer class to support grow-in-place for SRTP

Comment 6

[Eric Rescorla (:ekr)]

Comment on attachment 751220 [details] [diff] [review]
don't alloc/free on every packet send in MediaPipeline

Review of attachment 751220 [details] [diff] [review]:
-----------------------------------------------------------------

Seems like the DataBuffer aPI should be safer.

::: media/mtransport/databuffer.h
@@ +26,4 @@
>    }
>  
> +  void Assign(const uint8_t *data, size_t len, size_t size) {
> +    data_ = new unsigned char[ size ? size : 1];  // Don't depend on new [0].

This code should guarantee that size >= len

@@ +42,5 @@
> +      size_ = size;
> +    }
> +  }
> +
> +  void SetLength(size_t len) {

This should be a run-time check as well as an assert.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +563,5 @@
>    MOZ_ASSERT(pipeline_->rtp_transport_);
>    NS_ENSURE_TRUE(pipeline_->rtp_transport_, NS_ERROR_NULL_POINTER);
>  
> +  // libsrtp enciphers in place, so we need a big enough buffer.
> +  data->EnsureCapacity(data->len() + SRTP_MAX_EXPANSION);

Do we actually want to call this here? The buffer should always be big enough and
the ProtectRtp call will fail if it's not.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
@@ +135,5 @@
>     public:
>      // Implement the TransportInterface functions
>      PipelineTransport(MediaPipeline *pipeline)
>          : pipeline_(pipeline),
> +          sts_thread_(pipeline->sts_thread_) {}

What changed here?

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Attachment: don't alloc/free on every packet send in MediaPipeline

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 751577 [details] [diff] [review]
don't alloc/free on every packet send in MediaPipeline

The change to MediaPipeline.h was a tab -> spaces

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Attachment: don't alloc/free on every packet send in MediaPipeline

replace magic constant '20' with correct value (will verify with ASAN)

Comment 10

[Eric Rescorla (:ekr)]

Comment on attachment 754836 [details] [diff] [review]
don't alloc/free on every packet send in MediaPipeline

Review of attachment 754836 [details] [diff] [review]:
-----------------------------------------------------------------

This patch has bit-rotted because databuffer.h has changed.

Given the number of copies in the current code, I would be surprised
if this were a major contributor to CPU consumption, but if you feel
like unbitrotting it and resubmitting, I will TAL.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Attachment: don't alloc/free on every packet send in MediaPipeline

Unbitrotted.  Green Try: https://tbpl.mozilla.org/?tree=Try&rev=4beb0f7be284 (with some ASAN retriggers just for good measure)

Comment 12

[Martin Thomson [:mt:]]

Comment on attachment 8485909 [details] [diff] [review]
don't alloc/free on every packet send in MediaPipeline

Review of attachment 8485909 [details] [diff] [review]:
-----------------------------------------------------------------

I think that we should land this change.  After rebasing, that is.

::: media/mtransport/databuffer.h
@@ +35,4 @@
>      len_ = len;
>    }
>  
> +  void Allocate(size_t size) {

This should just call SetLength(0); EnsureCapacity(size);

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +863,5 @@
>    MOZ_ASSERT(pipeline_->rtp_.transport_);
>    NS_ENSURE_TRUE(pipeline_->rtp_.transport_, NS_ERROR_NULL_POINTER);
>  
> +  // libsrtp enciphers in place, so we need a big enough buffer.
> +  // Ensured by our only caller

This should assert that the size is genuinely large enough.  MOZ_ASSERT is verifiable; comments aren't.

@@ +874,3 @@
>      return res;
> +  }
> +  // don't need to SetLength() since we'll just delete this

Change to:
// Don't call data->SetLength(out_len), since we don't use the value.

Comment 13

[Maire Reavy [:mreavy]]

Randell -- Can you clean this up and get it reviewed & landed?

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Attachment: don't alloc/free on every packet send in MediaPipeline

unbitrotted, and cleaned up to remove the heavily-duplicated code pattern for RTP and RTCP

Comment 15

[Byron Campen [:bwc]]

Comment on attachment 8615988 [details] [diff] [review]
don't alloc/free on every packet send in MediaPipeline

Review of attachment 8615988 [details] [diff] [review]:
-----------------------------------------------------------------

Some small stuff, no need for another cycle.

::: media/mtransport/databuffer.h
@@ +16,5 @@
>  namespace mozilla {
>  
>  class DataBuffer {
>   public:
> +  DataBuffer() : data_(nullptr), len_(0), size_(0) {}

Suggest 'capacity' instead of 'size'.

@@ +48,5 @@
> +      size_ = size;
> +    }
> +  }
> +
> +  void SetLength(size_t len) {

Does anything use this? Also, should we allow this to increase len_?

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
@@ +191,5 @@
>      virtual nsresult SendRtcpPacket(const void* data, int len);
>  
>     private:
> +    virtual nsresult SendRtpRtcpPacket_s(nsAutoPtr<DataBuffer> data,
> +                                         bool is_rtp);

Not that you did this, but why is this virtual anyhow?

::: media/webrtc/signaling/src/mediapipeline/SrtpFlow.h
@@ +21,5 @@
>  #define SRTP_MASTER_KEY_LENGTH 16
>  #define SRTP_MASTER_SALT_LENGTH 14
>  #define SRTP_TOTAL_KEY_LENGTH (SRTP_MASTER_KEY_LENGTH + SRTP_MASTER_SALT_LENGTH)
>  
> +// SRTCP requires an auth tag *plus* a 4-byte index-plus-'E'-bit value (see RFC 3711)

Wrap to 80

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2cb094627289

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/564799379b68 for cpp unittest orange:

https://treeherder.mozilla.org/logviewer.html#?job_id=10495588&repo=mozilla-inbound

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7128670beeea

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

It was a minor issue with the transport_unittests relying on a side-effect that wasn't preserved (in an API that's  not used in non-unittest code).  Relanded

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7128670beeea