perf: optimize Request.read

[kevmoo]

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)
• 🧹 Code refactor
• ✅ Build configuration change
• 📝 Documentation
• 🗑️ Chore

[kevmoo]

@felangel – enable CI workflows?

[felangel]

@felangel – enable CI workflows?

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

[tomarra]

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.

[kevmoo]

@tomarra – done!

[kevmoo]

Do ya'll have benchmarks we could look at?

[tomarra]

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.

[alestiago]

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.

[kevmoo]

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

[felangel]

LGTM ✅

I ran some benchmarks locally and the Expando refactor does have a measurable performance impact.

For example, with a request body size of 1,071,472 bytes, and 100,000,000 calls to request.body, using Expando performed approximately 3% better (~500ms faster in the test I ran).

@kevmoo I expected to see a more significant performance improvement but since there is still a measurable performance gain and we're able to make the internal request final again, I think it's worth merging.

[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.

[felangel]

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 🎉