dburriss / event-feed

A HTTP based event feed for ASP.NET
MIT License
1 stars 1 forks source link

Serialization source gen. and property renames #3

Closed Grepsy closed 2 years ago

Grepsy commented 2 years ago

This PR contains two serialization related changes:

show up to 40% or more startup time reduction, private memory reduction, throughput speed increase

In a future PR I might also have a look at the nested serialization in the Payload property, if that makes sense (if we'll encrypt it won't).

dburriss commented 2 years ago

Use source gen

Awesome!

Use idiomatic naming

Do you know if the source gen then removes the slight perf hit around serializing/deserializing with mismatch casing? Honestly, though I just prefer "what you see is what you get" to messing around with options. Years of spending too much time going down NewtonSoft config means I am ok with my DTO looking exactly like the data it is transferring :) Not a strong preference though so if you can confirm there is no perf hit for the case change and attribute reflection (its all compile time now) then 👍 It is not something I have looked into a bunch.

This change did remind me that the _link was a pointer at HAL media type so I think we should commit to this and:

dburriss commented 2 years ago

@Grepsy Thanks for the PR!

2 points, the rest are just highlighting my point of view.

  1. Does the generated code nullify the perf hit taken for mismatched casing?
  2. Can we pull out a Serialize method that requires the context since it is required for correct functioning I don't want to use an optional parameter directly... prob makes more sense in the context so see comment ☝️
Grepsy commented 2 years ago

Thanks for the review and pointers!

I see your point about the decreased readability of the DTOs. My main reason for making the property-names conform to standard C# was because it was my impression we'd also be using these on the consumer side and exposing them to the application. Thinking as a consumer of this library I'd be annoyed when I have to work with ._links._tail public props :-) I think it's ok if they are purely internal to the producer (although it would still not be my preference).

One thing to note is the FeedEvent type though, which without any case-conversion is serialized as-is in PascalCase. This was the only reason the serializerOptions are needed. Without this change the response looks like this, which is mixing _this, camelCase and PascalCase. I'd like to propose to change the PascalCase to camelCase. I'll try and find the least intrusive way to do it so we can see whether that could work. Initial thought is adding extension methods that make sure the source generated overloads are always correctly used.

I can rename the properties to conform to HAL which should also help (only the _links uses the underscore). There also this standard we might consider instead of HAL because it includes paging and paging-metadata which seems to fit our usecase nicely.

{
            "EventId": "a2aff82b-f578-467a-a8f2-a1dde56d9c6b",
            "EventName": "test-event",
            ...
        }
    ],
    "_links": {
        "_meta": "/api/event-feed",
        "_head": "/api/event-feed/page/1",
        "_previous": "",
        "_self": "/api/event-feed/page/1",
        "_next": "/api/event-feed/page/2",
        "_tail": "/api/event-feed/page/2"
    },
    "isComplete": true
}

Regarding performance; the source generation doesn't incur a hit for doing the camelCase transformation. The propertynames seem to be compiled in and don't rely on reflection on runtime conversion.