Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RUMM-2786 Create AnyEncoder and AnyDecoder #1112

Merged
merged 2 commits into from
Jan 23, 2023
Merged

Conversation

maxep
Copy link
Member

@maxep maxep commented Jan 5, 2023

What and why?

Introduce implementation of AnyEncoder and AnyDecoder which will allow defragmenting data-types for transfer on the message-bus.

In the following example, a CrashContext is transmitted from the CrashReporting Feature to RUM through the message-bus:
image

This implementation will also be useful for the Flutter SDK to replace the dependency to DictionaryCoder. cc @fuzzybinary

Usage on the FeatureBaggage is done in #1113.

How?

Encoding

The AnyEncoder implements the Encoder protocol and is capable of encoding an Encodable object to a Any, [Any?], or [String: Any?] representation.
It will result in the same object as the following snippet, but without having to serialize to Data.

let encoder = JSONEncoder()
let data = try encoder.encode(value)
let object = try JSONSerialization.jsonObject(with: data, options: .allowFragments)

Decoding

The AnyDecoder implements the Decoder protocol and is capable of decoding aDecodable object from an Any? representation.
It will result in the same value as the following snippet, but without having to serialize to Data.

let decoder = JSONDecoder()
let data = try JSONSerialization.data(withJSONObject: object, options: [])
let value =  try decoder.decode(type, from: data)

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference
  • Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

  • Run unit tests
  • Run integration tests
  • Run smoke tests

@maxep maxep self-assigned this Jan 5, 2023
@maxep maxep requested a review from fuzzybinary January 5, 2023 14:07
@maxep maxep marked this pull request as ready for review January 6, 2023 13:56
@maxep maxep requested a review from a team as a code owner January 6, 2023 13:56
Copy link
Member

@maciejburda maciejburda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was curious about the performance of this solution and if measured correctly it appears that using JSONEncoder is still faster. Take a look at this example:

func measurePerformance() throws {
      let encoder = AnyEncoder()
      let jsonEncoder = JSONEncoder()

      struct Obj: Codable {
          let type: String = .mockAny()
          let id: String = .mockAny()
          let isFavorited: Bool = .mockAny()
          let attributes: Attributes = .init(title: .mockAny(), body: .mockAny())

          struct Attributes: Codable {
              let title: String
              let body: String
          }
      }
      let object = Obj()
      let iterations = 100_000
      let start = CFAbsoluteTimeGetCurrent()
      for _ in 1...iterations {
          let dict = try encoder.encode(object)
      }
      let diff = CFAbsoluteTimeGetCurrent() - start
      print("#1 Took \(diff) seconds")
      // #1 Took 2.4502270221710205 seconds


      let start2 = CFAbsoluteTimeGetCurrent()
      for _ in 1...iterations {
          let dict2 = try JSONSerialization.jsonObject(with: jsonEncoder.encode(object), options: .allowFragments)
      }
      let diff2 = CFAbsoluteTimeGetCurrent() - start2
      print("#2 Took \(diff2) seconds")
      // #2 Took 1.6677980422973633 seconds
  }

Sources/Datadog/DatadogInternal/Codable/AnyEncoder.swift Outdated Show resolved Hide resolved
@maxep maxep force-pushed the maxep/RUMM-2786/any-coder branch from cfee556 to e845c35 Compare January 17, 2023 13:40
@maxep maxep requested a review from a team as a code owner January 17, 2023 13:40
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Jan 17, 2023

Datadog Report

Branch report: maxep/RUMM-2786/any-coder
Commit report: 5abba31

dd-sdk-ios: 0 Failed, 0 New Flaky, 116 Passed, 0 Skipped, 4m 24.28s Wall Time

@maxep maxep force-pushed the maxep/RUMM-2786/any-coder branch from e845c35 to 5d54f09 Compare January 17, 2023 17:22
@maxep maxep changed the base branch from maxep/RUMM-2786/cr-bus-messages to develop January 17, 2023 17:27
@maxep
Copy link
Member Author

maxep commented Jan 17, 2023

Thanks for doing a quick benchmark, @maciejburda, it helped me find some optimizations. I'm not able to reach the combination of JSONEncoder and JSONSerialization (surprisingly), but I noticed that the decoding part is actually way more performant, see the snippet below. Since we need encoding+decoding on the bus, I consider this solution more performant overall. Additionally, the DictionaryEncodable protocol will help skip any serialization which is not possible with JSONEncoder.

let encoder = AnyEncoder()
let decoder = AnyDecoder()
let jsonEncoder = JSONEncoder()
let jsonDecoder = JSONDecoder()

struct Obj: Codable {
    let type: String = .mockAny()
    let id: String = .mockAny()
    let isFavorited: Bool = .mockAny()
    let attributes: Attributes = .init(title: .mockAny(), body: .mockAny())

    struct Attributes: Codable {
        let title: String
        let body: String
    }
}

let object = Obj()
let iterations = 100_000
let start = CFAbsoluteTimeGetCurrent()
for _ in 1...iterations {
    let dict = try encoder.encode(object)
    _ = try decoder.decode(Obj.self, from: dict)
}

let diff = CFAbsoluteTimeGetCurrent() - start
print("#1 Took \(diff) seconds")
// #1 Took 2.508360981941223 seconds

let start2 = CFAbsoluteTimeGetCurrent()
for _ in 1...iterations {
    let dict2 = try JSONSerialization.jsonObject(with: jsonEncoder.encode(object), options: .allowFragments)
    _ = try jsonDecoder.decode(Obj.self, from: JSONSerialization.data(withJSONObject: dict2, options: .fragmentsAllowed))
}
let diff2 = CFAbsoluteTimeGetCurrent() - start2
print("#2 Took \(diff2) seconds")
// #2 Took 3.418305993080139 seconds

@maxep maxep force-pushed the maxep/RUMM-2786/any-coder branch from 5d54f09 to 07b0d48 Compare January 17, 2023 18:14
@maxep
Copy link
Member Author

maxep commented Jan 18, 2023

I did further benchmarks:

Device: iPhone 11 (A13 Bionic chip), iOS 16.2

Model to serialize:

struct Model: Codable {
    var type: String = "type"
    var id: String = "id"
    var isFavorited: Bool = .random()
    var attributes: Attributes = .init()

    struct Attributes: Codable {
        var title: String = "title"
        var body: String = "body"
    }
}

Iterations: 100 000

Execution Time (in seconds)

AnyCoder JSONSerialization
encoding 0.9294730424880981 0.9025610685348511
decoding 1.0498170852661133 1.7138639688491821

CPU Profile (Weight)

AnyCoder JSONSerialization
encoding 19.6% 19.4%
decoding 22.8% 36.5%

Memory Allocation (Bytes Used)

AnyCoder JSONSerialization
encoding 1.62 KB 0.0% 3.47 KB 0.1%
decoding 1.34 KB 0.0% 32.25 KB 1.0%

Using such serialisation is obviously not free, but it performs better than the JSONCoder+JSONSerialization. I think this solution will drastically simplify our codebase by keeping a good separation of concerns between modules and keeping our flat dependency tree. Additionally, the DictionaryEncodable protocol is there to skip any serialisation of shared data types which will have a positive impact on perfs.

WDYT @ncreated @maciejburda ?

@maxep maxep requested review from maciejburda and ncreated January 18, 2023 09:57
Copy link
Member

@maciejburda maciejburda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks for the extra effort on optimising 💪

Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the idea 🚀⭐, and I believe it will be brilliant for passing events through message bus. The performance should not be a problem - thanks for taking care of that @maciejburda @maxep 👌🏎️ .

I left minor feedback on missing tests coverage and one proposal to IMO more convenient approach in testing.

Comment on lines +281 to +303
/// Stores a keyed encoding container for the given key and returns it.
///
/// - parameter keyType: The key type to use for the container.
/// - parameter key: The key to encode the container for.
/// - returns: A new keyed encoding container.
func nestedContainer<NestedKey>(keyedBy keyType: NestedKey.Type, forKey key: Key) -> KeyedEncodingContainer<NestedKey> where NestedKey: CodingKey {
let container = KeyedContainer<NestedKey>(
store: { self.set($0, forKey: key) },
path: codingPath + [key]
)
return KeyedEncodingContainer(container)
}

/// Stores an unkeyed encoding container for the given key and returns it.
///
/// - parameter key: The key to encode the container for.
/// - returns: A new unkeyed encoding container.
func nestedUnkeyedContainer(forKey key: Key) -> UnkeyedEncodingContainer {
UnkeyedContainer(
store: { self.set($0, forKey: key) },
path: codingPath + [key]
)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two have no coverage - seem to be not tested.

Comment on lines 54 to 72
func testObjectDecoding() throws {
let decoder = AnyDecoder()
let object = try decoder.decode(Object.self, from: dictionary)

XCTAssertEqual(object.id, id)
XCTAssertEqual(object.date, .mockAny())
XCTAssertEqual(object.title, "Response")
XCTAssertEqual(object.url, URL(string: "https://test.com/object/1"))
XCTAssertNotNil(object.nested)
XCTAssertEqual(object.nested.id, id)
XCTAssertEqual(object.int, 12_345)
XCTAssertNil(object.null)
XCTAssertTrue(object.bool ?? false)
XCTAssertNotNil(object.array)
XCTAssertEqual(object.array?.underestimatedCount, 5)
XCTAssertEqual(object.array?[0], AnyCodable(1))
}

func testObjectEncoding() throws {
Copy link
Member

@ncreated ncreated Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion/ These two tests ("object encoding" & "object encoding") seem quite tedious. They require maintaining both the struct Object and dictionary: [String: Any?] - making it easy to miss adding assertion. In fact, the assertions are already missing (I guess due to lack of convenience), e.g. we don't assert other elements in object.array than the first one:

XCTAssertEqual(object.array?.underestimatedCount, 5)
XCTAssertEqual(object.array?[0], AnyCodable(1))

What if, instead of testing "encoding" and "decoding" separately, we check the actual behavior (property) of the new concept we introduce in this PR? I can imagine we define two objects:

struct ActualObject: Codable {
   // various fields and nested type definitions
}

struct ExpectedObject: Codable {
   // same fields and types as in ActualObject
}

and do simple test with leveraging our existing RandomMockable and mirror comparison helpers:

func testEncodingAndDecoding() throws {
   let encoder = AnyEncoder()
   let decoder = AnyDecoder()
   
   // Given
   let expected: ExpectedObject = .mockRandom()

   // When
   let encoded = try encoder.encode(expected)
   let actual: ActualObject = try decoder.decode(from: encoded)

   // Then
   XCTAssertTrue(equalsAny(lhs: actual, rhs: expected))
}

Some benefits of this approach:

  • we will test against random values instead of hardcoded numbers or strings
  • covering more use cases won't require changing test code, but only ActualObject + ExpectedObject definitions
  • the rest of test bundle might benefit from any convenience helpers we need to add for it (e.g. one thing I spot is conforming AnyCodable to RandomMockable)
  • (IMHO) it documents this new concept pretty well

Downside is that assertions won't be isolated, so the equalsAny() will either pass or fail. This however can be improved by emitting proper error message in XCTFail() from equalsAny(lhs:rhs:).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes indeed 👍 I think we would need some changes with EquitableInTests. Let me see if I can include that or do another PR.

@maxep maxep force-pushed the maxep/RUMM-2786/any-coder branch from 07b0d48 to 5abba31 Compare January 20, 2023 16:28
@maxep maxep merged commit 6f0bcf2 into develop Jan 23, 2023
@maxep maxep deleted the maxep/RUMM-2786/any-coder branch January 23, 2023 12:55
@fuzzybinary fuzzybinary mentioned this pull request Feb 23, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants