dart_frog icon indicating copy to clipboard operation
dart_frog copied to clipboard
verified publisher icon VeryGoodOpenSource

chore: optimize Request.read

Open kevmoo opened this issue 1 year ago • 8 comments

Status

READY

Description

Use Expando instead of cloning each request.

  • Should improve performance. Should be measured.
  • Allows nested _request to be final. (Potentially allows future use of extension type

Type of Change

  • [ ] ✨ New feature (non-breaking change which adds functionality)
  • [ ] 🛠️ Bug fix (non-breaking change which fixes an issue)
  • [ ] ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • [x] 🧹 Code refactor
  • [ ] ✅ Build configuration change
  • [ ] 📝 Documentation
  • [ ] 🗑️ Chore

kevmoo avatar Jun 22 '24 00:06 kevmoo

@felangel – enable CI workflows?

kevmoo avatar Jun 22 '24 01:06 kevmoo

@felangel – enable CI workflows?

I’m not part of the organization anymore so I don’t have permissions unfortunately. @wolfenrain @alestiago can help 👍

felangel avatar Jun 22 '24 02:06 felangel

Hi @kevmoo 👋 Thanks for opening this PR. Would you be able to fill in the description of the PR with more details on what the goal of this change is and how it accomplishes that? I added our normal PR template back into the description so all you should need to do is fill out the description section with those details for us to do a proper review.

tomarra avatar Jun 25 '24 14:06 tomarra

@tomarra – done!

kevmoo avatar Jun 25 '24 19:06 kevmoo

Do ya'll have benchmarks we could look at?

kevmoo avatar Jun 29 '24 22:06 kevmoo

Do ya'll have benchmarks we could look at?

We do not. There are some benchmarks that are out there which you may be able to run locally, https://github.com/sharkbench/sharkbench comes to mind given their website https://sharkbench.dev/web/dart-dartfrog, to help validate this.

tomarra avatar Jul 01 '24 20:07 tomarra

I found this piece of documentation from the Flutter Style:

Generally speaking, Expando objects are a sign of an architectural problem. Carefully consider whether your usage is actually necessary. When your classes have clear documented ownership rules, there is usually a better solution.

Expando objects tend to invite code that is hard to understand because one cannot simply follow references to find all the dependencies.

This said, this usage could be an exception. Flutter itself still uses Expando in some occurrences.

alestiago avatar Mar 19 '25 10:03 alestiago

I think it depends on usage. I wouldn't land this PR unless there were performance tests run first!

kevmoo avatar Mar 20 '25 02:03 kevmoo

It was a bit of an experiment on my side to see if this pattern would work.

It'd LOVE to know MEMORY impact, too. Might be interesting to look at.

kevmoo avatar Sep 30 '25 07:09 kevmoo

100,000,000

Just took a quick look and the memory impact is much more significant in the test case I ran.

It was a bit of an experiment on my side to see if this pattern would work.

It'd LOVE to know MEMORY impact, too. Might be interesting to look at.

Just took a quick peak and memory impact seems to be more significant. I'll post some more detailed benchmarks/snapshots in a bit but thanks again for the contribution and glad we were finally able to land it 🎉

felangel avatar Sep 30 '25 15:09 felangel