Implement timing metrics for UrlRequest

components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.net.impl;
 
 import android.os.SystemClock;
 import android.support.annotation.Nullable;
 import android.util.Log;
 
@@ skipping 47 matching lines @@
 
     @GuardedBy("mUrlRequestAdapterLock")
     private boolean mStarted = false;
     @GuardedBy("mUrlRequestAdapterLock")
     private boolean mWaitingOnRedirect = false;
     @GuardedBy("mUrlRequestAdapterLock")
     private boolean mWaitingOnRead = false;
     @GuardedBy("mUrlRequestAdapterLock")
     @Nullable
     private final UrlRequestMetricsAccumulator mRequestMetricsAccumulator;
+    @GuardedBy("mUrlRequestAdapterLock")
+    private RequestFinishedInfo.Metrics mMetrics;
 
     /*
      * Synchronize access to mUrlRequestAdapter, mStarted, mWaitingOnRedirect,
      * and mWaitingOnRead.
      */
     private final Object mUrlRequestAdapterLock = new Object();
     private final CronetUrlRequestContext mRequestContext;
     private final Executor mExecutor;
 
     /*
@@ skipping 116 matching lines @@
     }
 
     @Override
     public void start() {
         synchronized (mUrlRequestAdapterLock) {
             checkNotStarted();
 
             try {
                 mUrlRequestAdapter =
                         nativeCreateRequestAdapter(mRequestContext.getUrlRequestContextAdapter(),
-                                mInitialUrl, mPriority, mDisableCache, mDisableConnectionMigration);
+                                mInitialUrl, mPriority, mDisableCache, mDisableConnectionMigration,
+                                mRequestContext.hasRequestFinishedListener());

[xunjieli, 2016/09/20 21:53:33]

Can we just use the metricsCollectionEnabled field from the constructor?
Grabbing the mUrlRequestAdapterLock and another lock makes me nervous about
deadlock. Since this is a best effort thing, I think it's fine to use
metricsCollectionEnabled from the constructor.

[mgersh, 2016/09/30 23:47:14]

I was actually planning on deleting that field, so we can do the check a bit
later. We're already doing the multiple locks thing elsewhere, with the same two
locks, so we'll have to always acquire them in the same order anyway. Added a
comment about this on mFinishedListenerLock.

[xunjieli, 2016/10/01 13:43:49]

Why do you plan to delete that field? We don't win a whole lot by delaying the
check from constructor to when start() is called. 
However, if you planning to remove the synchronized block from
CronetUrlRequestContext#createRequest(), then this is fine with me. Just making
sure there won't be two synchronized blocks trying to do the same thing.

[mgersh, 2016/10/04 21:14:58]

Mostly because I prefer not having that information stored in an excessive
number of places, and using metricsCollectionEnabled from the constructor would
require adding it as a member variable on CronetUrlRequest in addition to the
native request adapter and the list of listeners in the CronetUrlRequestContext.
But yes, that block is getting deleted.

[xunjieli, 2016/10/04 22:29:23]

Acknowledged.

                 mRequestContext.onRequestStarted();
                 if (mInitialMethod != null) {
                     if (!nativeSetHttpMethod(mUrlRequestAdapter, mInitialMethod)) {
                         throw new IllegalArgumentException("Invalid http method " + mInitialMethod);
                     }
                 }
 
                 boolean hasContentType = false;
                 for (Map.Entry<String, String> header : mRequestHeaders) {
                     if (header.getKey().equalsIgnoreCase("Content-Type")
@@ skipping 477 matching lines @@
     private void onStatus(final UrlRequest.StatusListener listener, final int loadState) {
         Runnable task = new Runnable() {
             @Override
             public void run() {
                 listener.onStatus(UrlRequest.Status.convertLoadState(loadState));
             }
         };
         postTaskToExecutor(task);
     }
 
+    /**
+     * Called by the native code to report metrics.
+     */
+    @SuppressWarnings("unused")
+    @CalledByNative
+    private void onMetricsCollected(long requestStartMs, long dnsStartMs, long dnsEndMs,
+            long connectStartMs, long connectEndMs, long sslStartMs, long sslEndMs,
+            long sendingStartMs, long sendingEndMs, long pushStartMs, long pushEndMs,
+            long responseStartMs, long responseEndMs, boolean socketReused, long sentBytesCount,
+            long receivedBytesCount) {
+        synchronized (mUrlRequestAdapterLock) {
+            if (mMetrics != null) {
+                throw new IllegalStateException("Metrics collection should only happen once.");
+            }
+            mMetrics = new CronetMetrics(requestStartMs, dnsStartMs, dnsEndMs, connectStartMs,
+                    connectEndMs, sslStartMs, sslEndMs, sendingStartMs, sendingEndMs, pushStartMs,
+                    pushEndMs, responseStartMs, responseEndMs, socketReused, sentBytesCount,
+                    receivedBytesCount);
+        }
+    }
+
     RequestFinishedInfo getRequestFinishedInfo() {
         // TODO(mgersh): fill in real values for finishedReason and exception
-        return new RequestFinishedInfo(mInitialUrl, mRequestAnnotations,
-                (mRequestMetricsAccumulator != null ? mRequestMetricsAccumulator.getRequestMetrics()
-                                                    : EMPTY_METRICS),
+        return new RequestFinishedInfo(mInitialUrl, mRequestAnnotations, mMetrics,
                 RequestFinishedInfo.SUCCEEDED, mResponseInfo, null);
     }
 
     private final class UrlRequestMetricsAccumulator {
         @Nullable
         private Long mRequestStartTime;
         @Nullable
         private Long mTtfbMs;
         @Nullable
         private Long mTotalTimeMs;
@@ skipping 27 matching lines @@
     /** Enforces prohibition of direct execution. */
     void checkCallingThread() {
         if (!mAllowDirectExecutor && mRequestContext.isNetworkThread(Thread.currentThread())) {
             throw new InlineExecutionProhibitedException();
         }
     }
 
     // Native methods are implemented in cronet_url_request_adapter.cc.
 
     private native long nativeCreateRequestAdapter(long urlRequestContextAdapter, String url,
-            int priority, boolean disableCache, boolean disableConnectionMigration);
+            int priority, boolean disableCache, boolean disableConnectionMigration,
+            boolean enableMetrics);
 
     @NativeClassQualifiedName("CronetURLRequestAdapter")
     private native boolean nativeSetHttpMethod(long nativePtr, String method);
 
     @NativeClassQualifiedName("CronetURLRequestAdapter")
     private native boolean nativeAddRequestHeader(long nativePtr, String name, String value);
 
     @NativeClassQualifiedName("CronetURLRequestAdapter")
     private native void nativeStart(long nativePtr);
 
     @NativeClassQualifiedName("CronetURLRequestAdapter")
     private native void nativeFollowDeferredRedirect(long nativePtr);
 
     @NativeClassQualifiedName("CronetURLRequestAdapter")
     private native boolean nativeReadData(
             long nativePtr, ByteBuffer byteBuffer, int position, int capacity);
 
     @NativeClassQualifiedName("CronetURLRequestAdapter")
     private native void nativeDestroy(long nativePtr, boolean sendOnCanceled);
 
     @NativeClassQualifiedName("CronetURLRequestAdapter")
     private native void nativeGetStatus(long nativePtr, UrlRequest.StatusListener listener);
 }