chore: optimize Request.read #1410
Conversation
|
@felangel – enable CI workflows? |
I’m not part of the organization anymore so I don’t have permissions unfortunately. @wolfenrain @alestiago can help 👍 |
| @@ -158,3 +154,5 @@ class Request { | |||
| return Request._(_request.change(headers: headers, path: path, body: body)); | |||
| } | |||
| } | |||
|
|
|||
| final _bodyFutureExpando = Expando<Future<String>>('dart_frog.request.body'); | |||
There was a problem hiding this comment.
Not that familiar with Expandos, would love to see a description on the PR to explain how this improves performance
There was a problem hiding this comment.
You all would have to measure it.
On the VM, they're pretty cheap. And now you're avoiding copying everything all the time which should be a lot cheaper
|
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. |
Co-authored-by: Jochum van der Ploeg <jochum@vdploeg.net>
Co-authored-by: Jochum van der Ploeg <jochum@vdploeg.net>
|
@tomarra – done! |
|
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. |
|
I found this piece of documentation from the Flutter Style:
This said, this usage could be an exception. Flutter itself still uses Expando in some occurrences. |
|
I think it depends on usage. I wouldn't land this PR unless there were performance tests run first! |
Status
READY
Description
Use
Expandoinstead of cloning each request._requestto be final. (Potentially allows future use ofextension typeType of Change