Skip to content

Commit

Permalink
Fabirc: Improvements in ImageResponseObserverCoordinator
Browse files Browse the repository at this point in the history
Summary:
This diff contains some small improvements in `ImageResponseObserverCoordinator` which are pretty minor:
 * Now we use `small_vector` instead of a normal one. In the vast majority of cases, the coordinator has only one observer, so having `small_vector` with default size `1` saves us a memory allocation (which is a dozen of allocations for a screen, not bad).
 * Empty constructor and destructor were removed.
 * Unnecessary copying of ImageResponse was removed. ImageResponse is a practically a shared_pointer, it has value semantic and does not need to be copied. We don't need to acquire mutex to access that.

Reviewed By: sammy-SC

Differential Revision: D17368740

fbshipit-source-id: 828e27a72b9c8ac0063c5fbda00f83ddb255309c
  • Loading branch information
shergin authored and facebook-github-bot committed Sep 23, 2019
1 parent bfc9839 commit 69f9fd4
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,6 @@
namespace facebook {
namespace react {

ImageResponseObserverCoordinator::ImageResponseObserverCoordinator() {
status_ = ImageResponse::Status::Loading;
}

ImageResponseObserverCoordinator::~ImageResponseObserverCoordinator() {}

void ImageResponseObserverCoordinator::addObserver(
ImageResponseObserver *observer) const {
ImageResponse::Status status = [this] {
Expand Down Expand Up @@ -51,7 +45,7 @@ void ImageResponseObserverCoordinator::removeObserver(

void ImageResponseObserverCoordinator::nativeImageResponseProgress(
float progress) const {
std::vector<ImageResponseObserver *> observersCopy = [this] {
auto observersCopy = [this] {
std::shared_lock<better::shared_mutex> read(mutex_);
return observers_;
}();
Expand All @@ -69,17 +63,13 @@ void ImageResponseObserverCoordinator::nativeImageResponseComplete(
status_ = ImageResponse::Status::Completed;
}

std::vector<ImageResponseObserver *> observersCopy = [this] {
auto observersCopy = [this] {
std::shared_lock<better::shared_mutex> read(mutex_);
return observers_;
}();

for (auto observer : observersCopy) {
ImageResponse imageResponseCopy = [this] {
std::unique_lock<better::shared_mutex> read(mutex_);
return ImageResponse(imageData_);
}();
observer->didReceiveImage(imageResponseCopy);
observer->didReceiveImage(imageResponse);
}
}

Expand All @@ -89,7 +79,7 @@ void ImageResponseObserverCoordinator::nativeImageResponseFailed() const {
status_ = ImageResponse::Status::Failed;
}

std::vector<ImageResponseObserver *> observersCopy = [this] {
auto observersCopy = [this] {
std::shared_lock<better::shared_mutex> read(mutex_);
return observers_;
}();
Expand Down
17 changes: 4 additions & 13 deletions ReactCommon/fabric/imagemanager/ImageResponseObserverCoordinator.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include <react/imagemanager/ImageResponseObserver.h>

#include <better/mutex.h>
#include <better/small_vector.h>
#include <vector>

namespace facebook {
Expand All @@ -24,16 +25,6 @@ namespace react {
*/
class ImageResponseObserverCoordinator {
public:
/*
* Default constructor.
*/
ImageResponseObserverCoordinator();

/*
* Default destructor.
*/
~ImageResponseObserverCoordinator();

/*
* Interested parties may observe the image response.
*/
Expand Down Expand Up @@ -66,19 +57,19 @@ class ImageResponseObserverCoordinator {
* List of observers.
* Mutable: protected by mutex_.
*/
mutable std::vector<ImageResponseObserver *> observers_;
mutable better::small_vector<ImageResponseObserver *, 1> observers_;

/*
* Current status of image loading.
* Mutable: protected by mutex_.
*/
mutable ImageResponse::Status status_;
mutable ImageResponse::Status status_{ImageResponse::Status::Loading};

/*
* Cache image data.
* Mutable: protected by mutex_.
*/
mutable std::shared_ptr<void> imageData_{};
mutable std::shared_ptr<void> imageData_;

/*
* Observer and data mutex.
Expand Down

0 comments on commit 69f9fd4

Please sign in to comment.