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

perf: Improve FormData #2560

Closed
wants to merge 0 commits into from
Closed

Conversation

tsctx
Copy link
Member

@tsctx tsctx commented Dec 29, 2023

Benchmark

Benchmark Script
import { bench, group, run } from 'mitata'
import { FormData } from '../lib/fetch/formdata.js'
import { FormData as FormDataCurrent } from '../lib/fetch/formdata2.js'
import symbols from '../lib/fetch/symbols.js'

const kState = symbols.kState

const data = [
  ['a', 'a'],
  ['b', 'b'],
  ['c', 'c'],
  ['d', 'd'],
  ['e', 'e'],
  ['f', 'f'],
  ['g', 'g'],
  ['h', 'h'],
  ['i', 'i']
]

/*
group('append FormData', () => {
  bench('current', () => {
    const fb = new FormDataCurrent()
    for (let i = 0; i < data.length; ++i) {
      fb.append(data[i][0], data[i][1])
    }
    return fb
  })
  bench('new', () => {
    const fb = new FormData()
    for (let i = 0; i < data.length; ++i) {
      fb.append(data[i][0], data[i][1])
    }
    return fb
  })
})
*/
group('set FormData', () => {
  bench('current', () => {
    const fb = new FormDataCurrent()
    for (let i = 0; i < data.length; ++i) {
      fb.set(data[i][0], data[i][1])
    }
    return fb
  })
  bench('new', () => {
    const fb = new FormData()
    for (let i = 0; i < data.length; ++i) {
      fb.set(data[i][0], data[i][1])
    }
    return fb
  })
})

group('set FormData (already append)', () => {
  const fb = new FormData()
  for (let i = 0; i < data.length; ++i) {
    fb.append(data[i][0], data[i][1])
  }

  const state = fb[kState]

  const stateLength = state.length

  function getNewFormDataCurrent () {
    const fb = new FormDataCurrent()
    fb[kState] = state.slice(0, stateLength)
    return fb
  }

  function getNewFormData () {
    const fb = new FormData()
    fb[kState] = state.slice(0, stateLength)
    return fb
  }
  bench('current', () => {
    const fb = getNewFormDataCurrent()
    for (let i = 0; i < data.length; ++i) {
      fb.set(data[i][0], data[i][1])
    }
    return fb
  })
  bench('new', () => {
    const fb = getNewFormData()
    for (let i = 0; i < data.length; ++i) {
      fb.set(data[i][0], data[i][1])
    }
    return fb
  })
})

group('get FormData', () => {
  const fbc = new FormDataCurrent()
  const fb = new FormData()
  for (let i = 0; i < data.length; ++i) {
    fbc.append(data[i][0], data[i][1])
    fb.append(data[i][0], data[i][1])
  }
  bench('current', () => {
    for (let i = 0; i < data.length; ++i) {
      fbc.get(data[i][0])
    }
    return fbc
  })
  bench('new', () => {
    for (let i = 0; i < data.length; ++i) {
      fb.get(data[i][0])
    }
    return fb
  })
})

group('getAll FormData', () => {
  const fbc = new FormDataCurrent()
  const fb = new FormData()
  for (let i = 0; i < data.length; ++i) {
    fbc.append(data[i][0], data[i][1])
    fb.append(data[i][0], data[i][1])
  }
  bench('current', () => {
    for (let i = 0; i < data.length; ++i) {
      fbc.getAll(data[i][0])
    }
    return fbc
  })
  bench('new', () => {
    for (let i = 0; i < data.length; ++i) {
      fb.getAll(data[i][0])
    }
    return fb
  })
})

group('has FormData', () => {
  const fbc = new FormDataCurrent()
  const fb = new FormData()
  for (let i = 0; i < data.length; ++i) {
    fbc.append(data[i][0], data[i][1])
    fb.append(data[i][0], data[i][1])
  }
  bench('current', () => {
    for (let i = 0; i < data.length; ++i) {
      fbc.has(data[i][0])
    }
    return fbc
  })
  bench('new', () => {
    for (let i = 0; i < data.length; ++i) {
      fb.has(data[i][0])
    }
    return fb
  })
})

group('delete FormData', () => {
  const fb = new FormData()
  for (let i = 0; i < data.length; ++i) {
    fb.append(data[i][0], data[i][1])
  }

  const state = fb[kState]

  const stateLength = state.length

  function getNewFormDataCurrent () {
    const fb = new FormDataCurrent()
    fb[kState] = state.slice(0, stateLength)
    return fb
  }

  function getNewFormData () {
    const fb = new FormData()
    fb[kState] = state.slice(0, stateLength)
    return fb
  }
  bench('current', () => {
    const fb = getNewFormDataCurrent()
    for (let i = 0; i < data.length; ++i) {
      fb.delete(data[i][0])
    }
    return fb
  })
  bench('new', () => {
    const fb = getNewFormData()
    for (let i = 0; i < data.length; ++i) {
      fb.delete(data[i][0])
    }
    return fb
  })
})

group('FormData forEach', () => {
  const fbc = new FormDataCurrent()
  const fb = new FormData()
  for (let i = 0; i < data.length; ++i) {
    fbc.append(data[i][0], data[i][1])
    fb.append(data[i][0], data[i][1])
  }
  bench('current', () => {
    const values = []
    fbc.forEach((v, k) => values.push([k, v]))
    return values
  })
  bench('new', () => {
    const values = []
    fb.forEach((v, k) => values.push([k, v]))
    return values
  })
})
/*
group('FormData@@iterator, () => {
  const fbc = new FormDataCurrent()
  const fb = new FormData()
  for (let i = 0; i < data.length; ++i) {
    fbc.append(data[i][0], data[i][1])
    fb.append(data[i][0], data[i][1])
  }
  bench('current', () => {
    return [...fbc]
  })
  bench('new', () => {
    return [...fb]
  })
})
*/
await new Promise((resolve) => setTimeout(resolve, 7000))

await run()
• set FormData
------------------------------------------------- -----------------------------
current      7.33 µs/iter      (5.6 µs … 3.77 ms)    6.6 µs   21.2 µs   24.8 µs
new          6.74 µs/iter     (5.6 µs … 827.3 µs)    6.4 µs   17.4 µs   20.4 µs

summary for set FormData
  new
   1.09x faster than current

• set FormData (already append)
------------------------------------------------- -----------------------------
current      9.64 µs/iter      (7.6 µs … 1.89 ms)    8.6 µs   29.5 µs   42.3 µs
new          7.46 µs/iter      (6.2 µs … 1.01 ms)    7.1 µs   19.5 µs   25.8 µs

summary for set FormData (already append)
  new
   1.29x faster than current

• get FormData
------------------------------------------------- -----------------------------
current    512.97 ns/iter (456.88 ns … 902.15 ns) 515.33 ns 884.24 ns 902.15 ns
new        494.85 ns/iter   (440.06 ns … 1.08 µs) 506.58 ns 913.63 ns   1.08 µs

summary for get FormData
  new
   1.04x faster than current

• getAll FormData
------------------------------------------------- -----------------------------
current       920 ns/iter      (700 ns … 1.12 ms)    900 ns      2 µs    2.3 µs
new        733.89 ns/iter   (675.26 ns … 1.14 µs) 708.49 ns   1.14 µs   1.14 µs

summary for getAll FormData
  new
   1.25x faster than current

• has FormData
------------------------------------------------- -----------------------------
current    485.17 ns/iter (444.11 ns … 985.48 ns) 466.07 ns 861.57 ns 985.48 ns
new        481.52 ns/iter (442.58 ns … 914.59 ns) 479.17 ns 897.94 ns 914.59 ns

summary for has FormData
  new
   1.01x faster than current

• delete FormData
------------------------------------------------- -----------------------------
current    776.43 ns/iter    (701.5 ns … 1.36 µs) 755.25 ns   1.36 µs   1.36 µs
new        687.18 ns/iter    (605.7 ns … 1.21 µs) 698.61 ns   1.21 µs   1.21 µs

summary for delete FormData
  new
   1.13x faster than current

• FormData forEach
------------------------------------------------- -----------------------------
current      6.51 µs/iter      (3.8 µs … 4.05 ms)    5.1 µs   20.5 µs   24.6 µs
new        151.51 ns/iter (126.69 ns … 494.79 ns) 150.45 ns 291.75 ns 383.37 ns

summary for FormData forEach
  new
   42.95x faster than current

@codecov-commenter
Copy link

codecov-commenter commented Dec 29, 2023

Codecov Report

Attention: 227 lines in your changes are missing coverage. Please review.

Comparison is base (e39a632) 85.54% compared to head (f07b204) 85.29%.
Report is 281 commits behind head on main.

Files Patch % Lines
lib/fetch/util.js 60.41% 57 Missing ⚠️
lib/fetch/index.js 69.36% 53 Missing ⚠️
lib/handler/RetryHandler.js 74.35% 30 Missing ⚠️
lib/cache/cache.js 12.12% 29 Missing ⚠️
lib/fetch/dataURL.js 79.31% 12 Missing ⚠️
lib/api/readable.js 83.92% 9 Missing ⚠️
lib/core/diagnostics.js 84.74% 9 Missing ⚠️
lib/eventsource/eventsource.js 96.09% 5 Missing ⚠️
lib/core/request.js 94.36% 4 Missing ⚠️
lib/fetch/formdata.js 90.47% 4 Missing ⚠️
... and 7 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2560      +/-   ##
==========================================
- Coverage   85.54%   85.29%   -0.26%     
==========================================
  Files          76       84       +8     
  Lines        6858     7624     +766     
==========================================
+ Hits         5867     6503     +636     
- Misses        991     1121     +130     

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

lib/fetch/formdata.js Outdated Show resolved Hide resolved
lib/fetch/formdata.js Outdated Show resolved Hide resolved
lib/fetch/formdata.js Outdated Show resolved Hide resolved
lib/fetch/formdata.js Outdated Show resolved Hide resolved
lib/fetch/formdata.js Outdated Show resolved Hide resolved
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

The benchmark results seem to indicate this PR is slower?

@tsctx
Copy link
Member Author

tsctx commented Dec 30, 2023

@ronag
Benchmarks updated and performance improved.
Can you take a look at it?

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

The for each results look suspicious. Either there is something wrong with the benchmark or then implementation.

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

I would argue that the performance improvements are too small relative to the maintenance cost.

@KhafraDev wdyt?

@KhafraDev
Copy link
Member

KhafraDev commented Dec 30, 2023

Yeah agreed. The forEach benchmarks are misleading because the implementation is now wrong (probably, I'd have to test it out). The WPTs must not cover inserting entries while iterating over it.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

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.

None yet

7 participants