Bug 1079653 (Part 1) - Move decode telemetry data from DecodeRequest to Decoder. r=tn
☠☠ backed out by a13927f78353 ☠ ☠
authorSeth Fowler <seth@mozilla.com>
Wed, 15 Oct 2014 13:52:21 -0700
changeset 210608 a8e760faf784c78986c0cbebede0177fd361ad4c
parent 210607 cbb710fac8064c79936b297889be4c3788ee00bd
child 210609 546f90c14465afd29b88fe16ff27bf5678b285d5
push id1
push userroot
push dateMon, 20 Oct 2014 17:29:22 +0000
reviewerstn
bugs1079653
milestone36.0a1
Bug 1079653 (Part 1) - Move decode telemetry data from DecodeRequest to Decoder. r=tn
image/src/Decoder.cpp
image/src/Decoder.h
image/src/RasterImage.cpp
image/src/RasterImage.h
--- a/image/src/Decoder.cpp
+++ b/image/src/Decoder.cpp
@@ -14,16 +14,17 @@
 namespace mozilla {
 namespace image {
 
 Decoder::Decoder(RasterImage &aImage)
   : mImage(aImage)
   , mCurrentFrame(nullptr)
   , mImageData(nullptr)
   , mColormap(nullptr)
+  , mChunkCount(0)
   , mDecodeFlags(0)
   , mBytesDecoded(0)
   , mDecodeDone(false)
   , mDataError(false)
   , mFrameCount(0)
   , mFailCode(NS_OK)
   , mNeedsNewFrame(false)
   , mInitialized(false)
@@ -92,20 +93,24 @@ Decoder::Write(const char* aBuffer, uint
     js::ProfileEntry::Category::GRAPHICS);
 
   MOZ_ASSERT(NS_IsMainThread() || aStrategy == DECODE_ASYNC);
 
   // We're strict about decoder errors
   MOZ_ASSERT(!HasDecoderError(),
              "Not allowed to make more decoder calls after error!");
 
+  // Begin recording telemetry data.
+  TimeStamp start = TimeStamp::Now();
+  mChunkCount++;
+
   // Keep track of the total number of bytes written.
   mBytesDecoded += aCount;
 
-  // If a data error occured, just ignore future data
+  // If a data error occured, just ignore future data.
   if (HasDataError())
     return;
 
   if (IsSizeDecode() && HasSize()) {
     // More data came in since we found the size. We have nothing to do here.
     return;
   }
 
@@ -117,16 +122,19 @@ Decoder::Write(const char* aBuffer, uint
   while (aStrategy == DECODE_SYNC && NeedsNewFrame() && !HasDataError()) {
     nsresult rv = AllocateFrame();
 
     if (NS_SUCCEEDED(rv)) {
       // Tell the decoder to use the data it saved when it asked for a new frame.
       WriteInternal(nullptr, 0, aStrategy);
     }
   }
+
+  // Finish telemetry.
+  mDecodeTime += (TimeStamp::Now() - start);
 }
 
 void
 Decoder::Finish(RasterImage::eShutdownIntent aShutdownIntent)
 {
   MOZ_ASSERT(NS_IsMainThread());
 
   // Implementation-specific finalization
--- a/image/src/Decoder.h
+++ b/image/src/Decoder.h
@@ -98,16 +98,22 @@ public:
   void SetObserver(imgDecoderObserver* aObserver)
   {
     MOZ_ASSERT(aObserver);
     mObserver = aObserver;
   }
 
   size_t BytesDecoded() const { return mBytesDecoded; }
 
+  // The amount of time we've spent inside Write() so far for this decoder.
+  TimeDuration DecodeTime() const { return mDecodeTime; }
+
+  // The number of times Write() has been called so far for this decoder.
+  uint32_t ChunkCount() const { return mChunkCount; }
+
   // The number of frames we have, including anything in-progress. Thus, this
   // is only 0 if we haven't begun any frames.
   uint32_t GetFrameCount() { return mFrameCount; }
 
   // The number of complete frames we have (ie, not including anything in-progress).
   uint32_t GetCompleteFrameCount() { return mInFrame ? mFrameCount - 1 : mFrameCount; }
 
   // Error tracking
@@ -230,16 +236,20 @@ protected:
   RefPtr<imgDecoderObserver> mObserver;
   ImageMetadata mImageMetadata;
 
   uint8_t* mImageData;       // Pointer to image data in either Cairo or 8bit format
   uint32_t mImageDataLength;
   uint32_t* mColormap;       // Current colormap to be used in Cairo format
   uint32_t mColormapSize;
 
+  // Telemetry data for this decoder.
+  TimeDuration mDecodeTime;
+  uint32_t mChunkCount;
+
   uint32_t mDecodeFlags;
   size_t mBytesDecoded;
   bool mDecodeDone;
   bool mDataError;
 
 private:
   uint32_t mFrameCount; // Number of frames, including anything in-progress
 
--- a/image/src/RasterImage.cpp
+++ b/image/src/RasterImage.cpp
@@ -2999,18 +2999,19 @@ RasterImage::FinishedSomeDecoding(eShutd
   // destroy it by destroying the decoder.
   nsRefPtr<RasterImage> image(this);
 
   bool done = false;
   bool wasSize = false;
   nsresult rv = NS_OK;
 
   if (image->mDecoder) {
-    if (request && request->mChunkCount && !image->mDecoder->IsSizeDecode()) {
-      Telemetry::Accumulate(Telemetry::IMAGE_DECODE_CHUNKS, request->mChunkCount);
+    if (!image->mDecoder->IsSizeDecode() && image->mDecoder->ChunkCount()) {
+      Telemetry::Accumulate(Telemetry::IMAGE_DECODE_CHUNKS,
+                            image->mDecoder->ChunkCount());
     }
 
     if (!image->mHasSize && image->mDecoder->HasSize()) {
       image->mDecoder->SetSizeOnImage();
     }
 
     // If the decode finished, or we're specifically being told to shut down,
     // tell the image and shut down the decoder.
@@ -3020,24 +3021,24 @@ RasterImage::FinishedSomeDecoding(eShutd
       // Hold on to a reference to the decoder until we're done with it
       nsRefPtr<Decoder> decoder = image->mDecoder;
 
       wasSize = decoder->IsSizeDecode();
 
       // Do some telemetry if this isn't a size decode.
       if (request && !wasSize) {
         Telemetry::Accumulate(Telemetry::IMAGE_DECODE_TIME,
-                              int32_t(request->mDecodeTime.ToMicroseconds()));
+                              int32_t(decoder->DecodeTime().ToMicroseconds()));
 
         // We record the speed for only some decoders. The rest have
         // SpeedHistogram return HistogramCount.
         Telemetry::ID id = decoder->SpeedHistogram();
         if (id < Telemetry::HistogramCount) {
           int32_t KBps = int32_t(decoder->BytesDecoded() /
-                                 (1024 * request->mDecodeTime.ToSeconds()));
+                                 (1024 * decoder->DecodeTime().ToSeconds()));
           Telemetry::Accumulate(id, KBps);
         }
       }
 
       // We need to shut down the decoder first, in order to ensure all
       // decoding routines have been finished.
       rv = image->ShutdownDecoder(aIntent);
       if (NS_FAILED(rv)) {
@@ -3441,54 +3442,47 @@ RasterImage::DecodePool::DecodeSomeOfIma
     // to read the size from most images.
     maxBytes = gfxPrefs::ImageMemDecodeBytesAtATime();
   }
 
   if (bytesToDecode == 0) {
     bytesToDecode = aImg->mSourceData.Length() - aImg->mDecoder->BytesDecoded();
   }
 
-  int32_t chunkCount = 0;
-  TimeStamp start = TimeStamp::Now();
-  TimeStamp deadline = start + TimeDuration::FromMilliseconds(gfxPrefs::ImageMemMaxMSBeforeYield());
+  TimeStamp deadline = TimeStamp::Now() +
+                       TimeDuration::FromMilliseconds(gfxPrefs::ImageMemMaxMSBeforeYield());
 
   // We keep decoding chunks until:
   //  * we don't have any data left to decode,
   //  * the decode completes,
   //  * we're an UNTIL_SIZE decode and we get the size, or
   //  * we run out of time.
   // We also try to decode at least one "chunk" if we've allocated a new frame,
   // even if we have no more data to send to the decoder.
   while ((aImg->mSourceData.Length() > aImg->mDecoder->BytesDecoded() &&
           bytesToDecode > 0 &&
           !aImg->IsDecodeFinished() &&
           !(aDecodeType == DECODE_TYPE_UNTIL_SIZE && aImg->mHasSize) &&
           !aImg->mDecoder->NeedsNewFrame()) ||
          (aImg->mDecodeRequest && aImg->mDecodeRequest->mAllocatedNewFrame)) {
-    chunkCount++;
     uint32_t chunkSize = std::min(bytesToDecode, maxBytes);
     nsresult rv = aImg->DecodeSomeData(chunkSize, aStrategy);
     if (NS_FAILED(rv)) {
       aImg->DoError();
       return rv;
     }
 
     bytesToDecode -= chunkSize;
 
     // Yield if we've been decoding for too long. We check this _after_ decoding
     // a chunk to ensure that we don't yield without doing any decoding.
     if (aDecodeType == DECODE_TYPE_UNTIL_TIME && TimeStamp::Now() >= deadline)
       break;
   }
 
-  if (aImg->mDecodeRequest) {
-    aImg->mDecodeRequest->mDecodeTime += (TimeStamp::Now() - start);
-    aImg->mDecodeRequest->mChunkCount += chunkCount;
-  }
-
   // Flush invalidations (and therefore paint) now that we've decoded all the
   // chunks we're going to.
   //
   // However, don't paint if:
   //
   //  * This was an until-size decode.  Until-size decodes are always followed
   //    by normal decodes, so don't bother painting.
   //
--- a/image/src/RasterImage.h
+++ b/image/src/RasterImage.h
@@ -326,17 +326,16 @@ private:
    * DecodeRequests.
    */
   struct DecodeRequest
   {
     explicit DecodeRequest(RasterImage* aImage)
       : mImage(aImage)
       , mBytesToDecode(0)
       , mRequestStatus(REQUEST_INACTIVE)
-      , mChunkCount(0)
       , mAllocatedNewFrame(false)
     {
       MOZ_ASSERT(aImage, "aImage cannot be null");
       MOZ_ASSERT(aImage->mStatusTracker,
                  "aImage should have an imgStatusTracker");
       mStatusTracker = aImage->mStatusTracker->CloneForRecording();
     }
 
@@ -354,23 +353,16 @@ private:
     {
       REQUEST_INACTIVE,
       REQUEST_PENDING,
       REQUEST_ACTIVE,
       REQUEST_WORK_DONE,
       REQUEST_STOPPED
     } mRequestStatus;
 
-    /* Keeps track of how much time we've burned decoding this particular decode
-     * request. */
-    TimeDuration mDecodeTime;
-
-    /* The number of chunks it took to decode this image. */
-    int32_t mChunkCount;
-
     /* True if a new frame has been allocated, but DecodeSomeData hasn't yet
      * been called to flush data to it */
     bool mAllocatedNewFrame;
 
   private:
     ~DecodeRequest() {}
   };