From e37708dfb605dd9ee9f4b2dac5d841d98b7d376c Mon Sep 17 00:00:00 2001 From: Paige Sun Date: Thu, 29 Oct 2020 21:57:46 -0700 Subject: [PATCH] Allow image loaders to enable/disable image telemetry Summary: When shouldEnableLoggingForRequestUrl is false, ImageTelemetry is not initialized, and no logging is done. * Replace `- (NSString *)loaderModuleNameForRequestUrl:(NSURL *)url` with `- (BOOL)shouldEnableLoggingForRequestUrl:(NSURL *)url` * Rename RCTImageLoaderInstrumentableProtocol.h -> RCTImageLoaderLoggableProtocol.h Reviewed By: fkgozali Differential Revision: D24523984 fbshipit-source-id: a5463eceea1c40f9452b0ad2ee6bf047f71a02c1 --- Libraries/Image/RCTImageLoader.h | 4 +-- Libraries/Image/RCTImageLoader.mm | 8 ++--- .../RCTImageLoaderInstrumentableProtocol.h | 15 ---------- Libraries/Image/RCTImageLoaderLoggable.h | 30 +++++++++++++++++++ .../RCTImageLoaderWithAttributionProtocol.h | 3 +- .../Image/RCTImageURLLoaderWithAttribution.h | 4 +-- .../renderer/imagemanager/ImageTelemetry.cpp | 8 ----- .../renderer/imagemanager/ImageTelemetry.h | 3 -- .../platform/ios/RCTImageManager.mm | 18 +++++------ 9 files changed, 48 insertions(+), 45 deletions(-) delete mode 100644 Libraries/Image/RCTImageLoaderInstrumentableProtocol.h create mode 100644 Libraries/Image/RCTImageLoaderLoggable.h diff --git a/Libraries/Image/RCTImageLoader.h b/Libraries/Image/RCTImageLoader.h index db660691353909..b7040e1a4c77d0 100644 --- a/Libraries/Image/RCTImageLoader.h +++ b/Libraries/Image/RCTImageLoader.h @@ -15,9 +15,9 @@ #import #import #import -#import +#import -@interface RCTImageLoader : NSObject +@interface RCTImageLoader : NSObject - (instancetype)init; - (instancetype)initWithRedirectDelegate:(id)redirectDelegate NS_DESIGNATED_INITIALIZER; - (instancetype)initWithRedirectDelegate:(id)redirectDelegate diff --git a/Libraries/Image/RCTImageLoader.mm b/Libraries/Image/RCTImageLoader.mm index 7c17250f1b0f82..a1962a92c4e9f7 100644 --- a/Libraries/Image/RCTImageLoader.mm +++ b/Libraries/Image/RCTImageLoader.mm @@ -839,12 +839,12 @@ - (RCTImageURLLoaderRequest *)loadImageWithURLRequest:(NSURLRequest *)imageURLRe return [[RCTImageURLLoaderRequest alloc] initWithRequestId:loaderRequest.requestId imageURL:imageURLRequest.URL cancellationBlock:cancellationBlock]; } -- (NSString *)loaderModuleNameForRequestUrl:(NSURL *)url { +- (BOOL)shouldEnablePerfLoggingForRequestUrl:(NSURL *)url { id loadHandler = [self imageURLLoaderForURL:url]; - if ([loadHandler respondsToSelector:@selector(loaderModuleNameForRequestUrl:)]) { - return [(id)loadHandler loaderModuleNameForRequestUrl:url]; + if ([loadHandler respondsToSelector:@selector(shouldEnablePerfLogging)]) { + return [(id)loadHandler shouldEnablePerfLogging]; } - return nil; + return NO; } - (void)trackURLImageVisibilityForRequest:(RCTImageURLLoaderRequest *)loaderRequest imageView:(UIView *)imageView diff --git a/Libraries/Image/RCTImageLoaderInstrumentableProtocol.h b/Libraries/Image/RCTImageLoaderInstrumentableProtocol.h deleted file mode 100644 index a803aa58daa417..00000000000000 --- a/Libraries/Image/RCTImageLoaderInstrumentableProtocol.h +++ /dev/null @@ -1,15 +0,0 @@ -/* - * Copyright (c) Facebook, Inc. and its affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -@protocol RCTImageLoaderInstrumentableProtocol - -/** -* Image instrumentation - get name of the image loader module -*/ -- (NSString *)loaderModuleNameForRequestUrl:(NSURL *)url; - -@end diff --git a/Libraries/Image/RCTImageLoaderLoggable.h b/Libraries/Image/RCTImageLoaderLoggable.h new file mode 100644 index 00000000000000..3b78c482bb5fa7 --- /dev/null +++ b/Libraries/Image/RCTImageLoaderLoggable.h @@ -0,0 +1,30 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * The image loader (i.e. RCTImageLoader) implement this to declare whether image performance should be logged. + */ +@protocol RCTImageLoaderLoggableProtocol + +/** + * Image instrumentation - declares whether its caller should log images + */ +- (BOOL)shouldEnablePerfLoggingForRequestUrl:(NSURL *)url; + +@end + +/** + * Image handlers in the image loader implement this to declare whether image performance should be logged. + */ +@protocol RCTImageLoaderLoggable + +/** + * Image instrumentation - declares whether its caller should log images + */ +- (BOOL)shouldEnablePerfLogging; + +@end diff --git a/Libraries/Image/RCTImageLoaderWithAttributionProtocol.h b/Libraries/Image/RCTImageLoaderWithAttributionProtocol.h index ee546a9b72357d..25dc6c04882cd8 100644 --- a/Libraries/Image/RCTImageLoaderWithAttributionProtocol.h +++ b/Libraries/Image/RCTImageLoaderWithAttributionProtocol.h @@ -9,7 +9,6 @@ #import #import -#import RCT_EXTERN BOOL RCTImageLoadingInstrumentationEnabled(void); RCT_EXTERN BOOL RCTImageLoadingPerfInstrumentationEnabled(void); @@ -19,7 +18,7 @@ RCT_EXTERN void RCTEnableImageLoadingPerfInstrumentation(BOOL enabled); RCT_EXTERN BOOL RCTGetImageLoadingPerfInstrumentationForFabricEnabled(); RCT_EXTERN void RCTSetImageLoadingPerfInstrumentationForFabricEnabledBlock(BOOL (^getEnabled)()); -@protocol RCTImageLoaderWithAttributionProtocol +@protocol RCTImageLoaderWithAttributionProtocol // TODO (T61325135): Remove C++ checks #ifdef __cplusplus diff --git a/Libraries/Image/RCTImageURLLoaderWithAttribution.h b/Libraries/Image/RCTImageURLLoaderWithAttribution.h index 4013367b4d3c63..ae6a333b26a296 100644 --- a/Libraries/Image/RCTImageURLLoaderWithAttribution.h +++ b/Libraries/Image/RCTImageURLLoaderWithAttribution.h @@ -7,7 +7,7 @@ #import #import -#import +#import // TODO (T61325135): Remove C++ checks #ifdef __cplusplus @@ -39,7 +39,7 @@ struct ImageURLLoaderAttribution { * Same as the RCTImageURLLoader interface, but allows passing in optional `attribution` information. * This is useful for per-app logging and other instrumentation. */ -@protocol RCTImageURLLoaderWithAttribution +@protocol RCTImageURLLoaderWithAttribution // TODO (T61325135): Remove C++ checks #ifdef __cplusplus diff --git a/ReactCommon/react/renderer/imagemanager/ImageTelemetry.cpp b/ReactCommon/react/renderer/imagemanager/ImageTelemetry.cpp index 7e754307feca7c..43a6134495ecdb 100644 --- a/ReactCommon/react/renderer/imagemanager/ImageTelemetry.cpp +++ b/ReactCommon/react/renderer/imagemanager/ImageTelemetry.cpp @@ -19,14 +19,6 @@ SurfaceId ImageTelemetry::getSurfaceId() const { return surfaceId_; } -std::string ImageTelemetry::getLoaderModuleName() const { - return loaderModuleName_; -} - -void ImageTelemetry::setLoaderModuleName(std::string const &loaderModuleName) { - loaderModuleName_ = loaderModuleName; -} - TelemetryTimePoint ImageTelemetry::getWillRequestUrlTime() const { assert(willRequestUrlTime_ != kTelemetryUndefinedTimePoint); return willRequestUrlTime_; diff --git a/ReactCommon/react/renderer/imagemanager/ImageTelemetry.h b/ReactCommon/react/renderer/imagemanager/ImageTelemetry.h index 312d19b19e9862..3f053dbcf3ea1a 100644 --- a/ReactCommon/react/renderer/imagemanager/ImageTelemetry.h +++ b/ReactCommon/react/renderer/imagemanager/ImageTelemetry.h @@ -31,14 +31,11 @@ class ImageTelemetry final { TelemetryTimePoint getWillRequestUrlTime() const; SurfaceId getSurfaceId() const; - std::string getLoaderModuleName() const; - void setLoaderModuleName(std::string const &loaderModuleName); private: TelemetryTimePoint willRequestUrlTime_{kTelemetryUndefinedTimePoint}; const SurfaceId surfaceId_; - std::string loaderModuleName_{""}; }; } // namespace react diff --git a/ReactCommon/react/renderer/imagemanager/platform/ios/RCTImageManager.mm b/ReactCommon/react/renderer/imagemanager/platform/ios/RCTImageManager.mm index b0062c9c192e62..3fa8c36b291746 100644 --- a/ReactCommon/react/renderer/imagemanager/platform/ios/RCTImageManager.mm +++ b/ReactCommon/react/renderer/imagemanager/platform/ios/RCTImageManager.mm @@ -40,8 +40,15 @@ - (ImageRequest)requestImage:(ImageSource)imageSource surfaceId:(SurfaceId)surfa { SystraceSection s("RCTImageManager::requestImage"); - auto telemetry = std::make_shared(surfaceId); - telemetry->willRequestUrl(); + NSURLRequest *request = NSURLRequestFromImageSource(imageSource); + std::shared_ptr telemetry; + if ([self->_imageLoader shouldEnablePerfLoggingForRequestUrl:request.URL]) { + telemetry = std::make_shared(surfaceId); + telemetry->willRequestUrl(); + } else { + telemetry = nullptr; + } + auto imageRequest = ImageRequest(imageSource, telemetry); auto weakObserverCoordinator = (std::weak_ptr)imageRequest.getSharedObserverCoordinator(); @@ -49,13 +56,6 @@ - (ImageRequest)requestImage:(ImageSource)imageSource surfaceId:(SurfaceId)surfa auto sharedCancelationFunction = SharedFunction<>(); imageRequest.setCancelationFunction(sharedCancelationFunction); - NSURLRequest *request = NSURLRequestFromImageSource(imageSource); - BOOL hasModuleName = [self->_imageLoader respondsToSelector:@selector(loaderModuleNameForRequestUrl:)]; - NSString *moduleName = hasModuleName ? [self->_imageLoader loaderModuleNameForRequestUrl:request.URL] : nil; - std::string moduleCString = - std::string([moduleName UTF8String], [moduleName lengthOfBytesUsingEncoding:NSUTF8StringEncoding]); - telemetry->setLoaderModuleName(moduleCString); - /* * Even if an image is being loaded asynchronously on some other background thread, some other preparation * work (such as creating an `NSURLRequest` object and some obscure logic inside `RCTImageLoader`) can take a couple