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

fetch: limit web streams usage in body mixin methods #3113

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mertcanaltin
Copy link
Member

@mertcanaltin mertcanaltin commented Apr 13, 2024

This PR aims to improve performance by optimizing the processing of webflows in the fetch module.
This experiment was already done by @KhafraDev , I just opened a pr

I thought there would be discussion about it and it might benefit fetch πŸš€
#2164
here is a benchmark made by @KhafraDev πŸ™
benchmarks:

import { Response as UndiciResponse } from './index.js'
import { cronometro } from 'cronometro'

await cronometro({
  async 'undici Response' () {
    await new UndiciResponse('abc').text()
  },
  async 'global Response' () {
    await new Response('abc').text()
  }
})
╔═════════════════╀═════════╀═════════════════╀═══════════╗
β•‘ Slower tests    β”‚ Samples β”‚          Result β”‚ Tolerance β•‘
β•Ÿβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”Όβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”Όβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”Όβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β•’
β•‘ global Response β”‚   10000 β”‚ 40169.19 op/sec β”‚  Β± 1.59 % β•‘
β•Ÿβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”Όβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”Όβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”Όβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β•’
β•‘ Fastest test    β”‚ Samples β”‚          Result β”‚ Tolerance β•‘
β•Ÿβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”Όβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”Όβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”Όβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β•’
β•‘ undici Response β”‚   10000 β”‚ 44024.31 op/sec β”‚  Β± 2.12 % β•‘
β•šβ•β•β•β•β•β•β•β•β•β•β•β•β•β•β•β•β•β•§β•β•β•β•β•β•β•β•β•β•§β•β•β•β•β•β•β•β•β•β•β•β•β•β•β•β•β•β•§β•β•β•β•β•β•β•β•β•β•β•β•

@mertcanaltin
Copy link
Member Author

I'm running tests, I'll fix them πŸ§ͺ

@@ -1055,6 +1055,11 @@ async function fullyReadBody (body, processBody, processBodyError) {
// with taskDestination.
const errorSteps = processBodyError

if (typeof body.source === 'string') {
successSteps(new TextEncoder().encode(body.source))
Copy link
Contributor

@Uzlopak Uzlopak Apr 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not check your code and if this makes sense. But I think this wil break without-intl builds . check how we implemented utf8Decode and look if you can implement something like utf8Encode?!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you very much for your suggestion I will try this

lib/web/fetch/util.js Outdated Show resolved Hide resolved
Co-authored-by: tsctx <91457664+tsctx@users.noreply.github.com>
@KhafraDev
Copy link
Member

There are related failures, but I don't necessarily think this should land as-is even if those failures were fixed.

Copy link
Member

@KhafraDev KhafraDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(making sure to block this)

@mertcanaltin
Copy link
Member Author

I wonder if you have a suggestion, if it's healthy to keep it as it is, I'll close this place @KhafraDev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants