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

feat: update InterfaceStubGenerator to be an incremental generator #1864

Merged
merged 1 commit into from
Oct 10, 2024

Conversation

TimothyMakkison
Copy link
Contributor

@TimothyMakkison TimothyMakkison commented Oct 6, 2024

Closes #1791

  • Parser converts each INamedTypeSymbol into InterfaceModel and extracts additional information.
  • All Model types should be structurally equatable to enable incremental generation.
    • ImmutableEquatableArray is a structurally equatable collection.
  • Emitter takes each model and creates an interface stub.
  • I’ve tried to keep the logic and structure as close to the original as I could to prevent errors.

TODO

  • Add more tests
  • Delete unused code
  • Need to do a thorough comparison between the old logic and new to ensure that I haven't made a translation error.

Future changes

  • Remove some of the unneeded ToList calls
  • Optimize the model memory usage. ie ImmutableEquatableArray could be made into a struct.
  • I'd like to copy System.Text.Json and rename files to InterfaceStubGenerator.Parser etc

Benchmarks

Original

Method Mean Error StdDev Gen0 Gen1 Allocated
Compile 249.6 us 4.97 us 6.46 us 12.6953 2.9297 117.52 KB
Cached 216.1 us 4.10 us 4.02 us 12.2070 2.9297 114.86 KB
CompileMany 8,898.8 us 171.75 us 197.78 us 687.5000 343.7500 6354.62 KB
CachedMany 7,897.2 us 139.81 us 137.32 us 671.8750 328.1250 6289.45 KB

Incremental

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
Compile 215.4 us 3.76 us 3.69 us 12.2070 1.4648 - 115.5 KB
Cached 158.1 us 2.97 us 2.91 us 5.8594 0.2441 - 54.44 KB
CompileMany 8,142.1 us 138.00 us 129.08 us 765.6250 296.8750 31.2500 6793.62 KB
CachedMany 5,173.9 us 97.88 us 100.52 us 156.2500 54.6875 - 1455.1 KB

Copy link

codecov bot commented Oct 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.80%. Comparing base (6ebeda5) to head (59910d7).
Report is 135 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1864      +/-   ##
==========================================
- Coverage   87.73%   83.80%   -3.93%     
==========================================
  Files          33       36       +3     
  Lines        2348     2476     +128     
  Branches      294      347      +53     
==========================================
+ Hits         2060     2075      +15     
- Misses        208      318     +110     
- Partials       80       83       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ChrisPulman
Copy link
Member

Hi @TimothyMakkison , do you think you could try to update dependency verify.sourcegenerators to 2.5.0 along with this PR or as a separate PR.
I think I managed to conflict with one of your updates with some of my PR's, hence the conflict, sorry.
So far this is looking good, thank you, I have updated the Minor, however this looks like a Major version change; I think we can wait until this section of work is complete before releasing again, please let me know when you think the code is ready to release should be a nice update by the looks of things. If you think it can stay with a minor update then do let me know.

@TimothyMakkison
Copy link
Contributor Author

TimothyMakkison commented Oct 6, 2024

Hi @TimothyMakkison , do you think you could try to update dependency verify.sourcegenerators to 2.5.0 along with this PR or as a separate PR.

Sure, I don't think changing this should affect me.

I think I managed to conflict with one of your updates with some of my PR's, hence the conflict, sorry.

nw, it looks like most of the changes can be copied over or ignored, shouldn't be a problem.

So far this is looking good, thank you, I have updated the Minor, however this looks like a Major version change; I think we can wait until this section of work is complete before releasing again, please let me know when you think the code is ready to release should be a nice update by the looks of things. If you think it can stay with a minor update then do let me know.

Thanks 😄, in theory this should be a fully backwards compatible change and will only provide a smoother ide experience for users. Assuming that I haven't broken anything🤞. I'm not too familiar with versioning so I'm happy to leave the decision to you. Does refit have a pre release version?

I was hoping to finish this in a couple of days, I need to do some cleanup, re-read Parser/Emitter a couple of times and probably write more tests. I'll mark this as ready for review when ready.

@TimothyMakkison TimothyMakkison force-pushed the Incrementak_test branch 2 times, most recently from a56e5d3 to 4729184 Compare October 7, 2024 21:35
@TimothyMakkison TimothyMakkison force-pushed the Incrementak_test branch 4 times, most recently from eab1de1 to d532015 Compare October 10, 2024 15:22
@TimothyMakkison
Copy link
Contributor Author

TimothyMakkison commented Oct 10, 2024

Should be sound 🤞 . I tried to keep the logic the same and do a straight translation from the original, however there are differences. Note that bools passed to ParseMethod may appear to be inverted as the its logic is a mix of ProcessRefitMethod and WriteMethodOpening.

I'll probably look into #1801 or #1389 next. I could probably do #1801 over the weekend if you want it in the next update.
#1389 will take a lot longer, should I create a separate issue for it?

@TimothyMakkison TimothyMakkison marked this pull request as ready for review October 10, 2024 16:04
@ChrisPulman
Copy link
Member

Shall we get 1801 out the way first as it's producing errors in the code, then tackle 1389

@ChrisPulman ChrisPulman merged commit 26cfb28 into reactiveui:main Oct 10, 2024
2 of 3 checks passed
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.

Improve incremental generator caching
2 participants