Port FieldTransform to C++ #1033

zxu123 · 2018-04-05T17:52:32Z

Discussion

This is part of the C++ migration.

Testing

unit and integration test pass.

API Changes

n/a

…ransform) to C++

zxu123 · 2018-04-05T17:54:07Z

The true commits start from 8eecd35. (The earlier ones are from base branch)

var-const · 2018-04-05T19:11:00Z

Firestore/Example/Tests/Util/FSTHelpers.mm

@@ -278,15 +281,14 @@ NSComparator FSTTestDocComparator(const absl::string_view fieldPath) {
 FSTTransformMutation *FSTTestTransformMutation(NSString *path,
                                               NSArray<NSString *> *serverTimestampFields) {
  FSTDocumentKey *key = [FSTDocumentKey keyWithPath:testutil::Resource(util::MakeStringView(path))];
-  NSMutableArray<FSTFieldTransform *> *fieldTransforms = [NSMutableArray array];
+  std::vector<FieldTransform> fieldTransforms{};


Nit: {} is unnecessary.

var-const · 2018-04-05T19:11:50Z

Firestore/Example/Tests/Util/FSTHelpers.mm

@@ -278,15 +281,14 @@ NSComparator FSTTestDocComparator(const absl::string_view fieldPath) {
 FSTTransformMutation *FSTTestTransformMutation(NSString *path,
                                               NSArray<NSString *> *serverTimestampFields) {
  FSTDocumentKey *key = [FSTDocumentKey keyWithPath:testutil::Resource(util::MakeStringView(path))];
-  NSMutableArray<FSTFieldTransform *> *fieldTransforms = [NSMutableArray array];
+  std::vector<FieldTransform> fieldTransforms{};
+  fieldTransforms.reserve(serverTimestampFields.count);


Nit: unless the number is expected to be pretty large, this is probably overkill.

var-const · 2018-04-05T19:14:00Z

Firestore/core/src/firebase/firestore/model/field_transform.h

+  }
+
+#if defined(__OBJC__)
+  bool operator==(const FieldTransform& other) const {


Equality operator seems like a reasonable operation to have, maybe move outside the #if section?

var-const · 2018-04-05T19:14:48Z

Firestore/core/src/firebase/firestore/model/field_transform.h

+ private:
+  FieldPath path_;
+  // Shared by copies of the same FieldTransform.
+  std::shared_ptr<TransformOperation> transform_;


Can it be <const TransformOperation>? Also, can you please provide more context for this optimization?

Change to const. Really does not matter for the only ServerTimestampTransform. I think we can safely assume a TransformOperation will not mutate itself when proceeding fields.

We need to access a TransformOperation here. Since it is poly, we need either a pointer or a reference. Conceptually, FieldTransform should own it instead of each TransformOperation type owns it (for ServingTimestampTransform might be OK since it builds a singleton). Now if you wonder why unique_ptr does not work here, that's because a FieldTransform object can be copied.

I think we can safely assume a TransformOperation will not mutate itself when proceeding fields.

Thanks for making the change. I think it's useful to enforce such assumptions using the type system. Shared ownership of immutable data is a much simpler model than shared ownership of mutable data.

Now if you wonder why unique_ptr does not work here, that's because a FieldTransform object can be copied.

It's possible to do something like:

FieldTransform(const FieldTransform& rhs) : transform_{rhs.transform_.Clone()} {}

But it's more hassle -- as long as shared_ptr points to const, I'm fine with it.

var-const · 2018-04-05T19:15:59Z

Firestore/core/src/firebase/firestore/model/field_transform.h

+    return path_;
+  }
+
+  const TransformOperation* transform() const {


Nit: the function name sounds like an action (i.e., "do apply transformation"). transformation? transform_operation?

var-const · 2018-04-05T19:18:12Z

Firestore/Source/Remote/FSTSerializerBeta.mm

    (NSArray<GCFSDocumentTransform_FieldTransform *> *)protos {
-  NSMutableArray<FSTFieldTransform *> *fieldTransforms = [NSMutableArray array];
+  std::vector<FieldTransform> fieldTransforms{};


Same comments on vector initialization.

var-const · 2018-04-05T19:20:02Z

Firestore/Source/Model/FSTMutation.mm

  return result;
 }

 - (NSString *)description {
-  return [NSString stringWithFormat:@"<FSTTransformMutation key=%s transforms=%@ precondition=%@>",
-                                    self.key.ToString().c_str(), self.fieldTransforms,
+  std::string fieldTransforms = "";


Nit: = "" is unnecessary.

var-const · 2018-04-05T19:24:31Z

Firestore/Source/API/FSTUserDataConverter.mm

-  // _fieldMask is shared across all active context objects to accumulate the result. For example,
-  // the result of calling any of contextForField, contextForFieldPath and contextForArrayIndex
-  // shares the ownership of _fieldMask.
+  // _fieldMask and _fieldTransforms are shared across all active context objects to accumulate the


I guess alternatively, a single context object could be wrapped in a shared_ptr. Can you please provide some context on how it was implemented in Objective-C?

In ObjC, NSMutableArray* is used. It is not a single FSTParseContext object is shared by many users. It is multiple FSTParseContext objects will update the same (shared) fieldMask and fieldTransforms. From my limited reading on the codes, it is mainly due to some recursive parse of the serialized stream input, where each sub-structure has their own FSTParseContext object but all put the parsed result into the shared fieldMask and fieldTransforms. I am confident there is alternative design where there is separate class for those shared part, say SharedParsedContext. But I'd not do the refactor in this porting PR.

Thanks for the explanation.

var-const · 2018-04-10T15:17:12Z

Firestore/core/src/firebase/firestore/model/field_transform.h

+ private:
+  FieldPath path_;
+  // Shared by copies of the same FieldTransform.
+  std::shared_ptr<TransformOperation> transform_;


I think we can safely assume a TransformOperation will not mutate itself when proceeding fields.

Thanks for making the change. I think it's useful to enforce such assumptions using the type system. Shared ownership of immutable data is a much simpler model than shared ownership of mutable data.

Now if you wonder why unique_ptr does not work here, that's because a FieldTransform object can be copied.

It's possible to do something like:

FieldTransform(const FieldTransform& rhs) : transform_{rhs.transform_.Clone()} {}

But it's more hassle -- as long as shared_ptr points to const, I'm fine with it.

var-const · 2018-04-10T15:19:17Z

Firestore/Source/API/FSTUserDataConverter.mm

-  // _fieldMask is shared across all active context objects to accumulate the result. For example,
-  // the result of calling any of contextForField, contextForFieldPath and contextForArrayIndex
-  // shares the ownership of _fieldMask.
+  // _fieldMask and _fieldTransforms are shared across all active context objects to accumulate the


Thanks for the explanation.

wilhuff

Pretty much LGTM

wilhuff · 2018-04-10T19:12:50Z

Firestore/Source/Model/FSTMutation.mm

-    FSTFieldTransform *fieldTransform = self.fieldTransforms[i];
-    const TransformOperation *transform = fieldTransform.transform;
-    FieldPath fieldPath = fieldTransform.path;
+  for (NSUInteger i = 0; i < self.fieldTransforms.size(); i++) {


The loop variable here should now be size_t.

wilhuff · 2018-04-10T19:16:40Z

Firestore/core/src/firebase/firestore/model/field_transform.h

+ public:
+  FieldTransform(FieldPath path,
+                 std::unique_ptr<TransformOperation> transformation)
+      : path_(std::move(path)), transformation_(std::move(transformation)) {


Prefer brace initialization for consistency.

wilhuff · 2018-04-10T19:17:14Z

Firestore/core/src/firebase/firestore/model/field_transform.h

+class FieldTransform {
+ public:
+  FieldTransform(FieldPath path,
+                 std::unique_ptr<TransformOperation> transformation)


Move constructors should be marked noexcept, no?

wilhuff · 2018-04-10T19:18:06Z

Firestore/core/src/firebase/firestore/model/field_transform.h

+    return path_;
+  }
+
+  const TransformOperation* transformation() const {


I realize that inside FieldTransform we need the TransformOperation to be stored in a unique_ptr so that we can store different subtypes. However, it seems like this implementation detail does not need to leak outside FieldTransform and this method could return a const TransformOperation& instead of a const pointer.

That's true. Fixed.

zxu123

Updated.

zxu123 · 2018-04-10T19:23:42Z

Firestore/Source/Model/FSTMutation.mm

-    FSTFieldTransform *fieldTransform = self.fieldTransforms[i];
-    const TransformOperation *transform = fieldTransform.transform;
-    FieldPath fieldPath = fieldTransform.path;
+  for (NSUInteger i = 0; i < self.fieldTransforms.size(); i++) {


zxu123 · 2018-04-10T19:24:46Z

Firestore/core/src/firebase/firestore/model/field_transform.h

+ public:
+  FieldTransform(FieldPath path,
+                 std::unique_ptr<TransformOperation> transformation)
+      : path_(std::move(path)), transformation_(std::move(transformation)) {


zxu123 · 2018-04-10T19:25:57Z

Firestore/core/src/firebase/firestore/model/field_transform.h

+class FieldTransform {
+ public:
+  FieldTransform(FieldPath path,
+                 std::unique_ptr<TransformOperation> transformation)


zxu123 · 2018-04-10T19:27:10Z

Firestore/core/src/firebase/firestore/model/field_transform.h

+    return path_;
+  }
+
+  const TransformOperation* transformation() const {


That's true. Fixed.

wilhuff

LGTM

* port FieldMask to C++ * address changes * address changes * fix test * address change * Port transform operations (FSTTransformOperation, FSTServerTimestampTransform) to C++ * address changes * address changes * address changes * implement `FieldTransform` in C++ * port `FieldTransform` * make `fieldTransforms` shared inside `context` * address changes * fix lint * address changes

zxu123 added 14 commits March 29, 2018 15:59

port FieldMask to C++

96a64b3

address changes

c6ee4b0

address changes

64cfaac

fix test

d27dc11

address change

235bc20

Port transform operations (FSTTransformOperation, FSTServerTimestampT…

a97ef14

…ransform) to C++

address changes

6d47fe4

address changes

9ea9ab2

address changes

f25eebc

Merge branch 'master' into cpp/port_transform

a579ba5

implement FieldTransform in C++

8eecd35

port FieldTransform

b4c78da

Merge branch 'master' into cpp/port_field_transform

84e85d4

make fieldTransforms shared inside context

78a8bb9

googlebot added the cla: yes label Apr 5, 2018

zxu123 requested review from var-const and rsgowman April 5, 2018 17:52

zxu123 added the api: firestore label Apr 5, 2018

var-const reviewed Apr 5, 2018

View reviewed changes

zxu123 added 3 commits April 6, 2018 16:06

address changes

a87cb0a

Merge branch 'master' into cpp/port_field_transform

0991762

Merge branch 'master' into cpp/port_field_transform

93cf65a

var-const approved these changes Apr 10, 2018

View reviewed changes

fix lint

e6edf9a

zxu123 requested a review from wilhuff April 10, 2018 16:37

wilhuff reviewed Apr 10, 2018

View reviewed changes

address changes

ab083c5

zxu123 commented Apr 10, 2018

View reviewed changes

wilhuff approved these changes Apr 10, 2018

View reviewed changes

zxu123 merged commit 2ac778f into master Apr 10, 2018

zxu123 deleted the cpp/port_field_transform branch April 10, 2018 20:36

firebase locked and limited conversation to collaborators Nov 5, 2019

Port FieldTransform to C++ #1033

Port FieldTransform to C++ #1033

Conversation

zxu123 commented Apr 5, 2018

Discussion

Testing

API Changes

zxu123 commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilhuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zxu123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilhuff left a comment

Choose a reason for hiding this comment