fix(lib/utils): simpler algorithm for removeDotSegments

[wooffie]

Replace regex to linear algorithm. This fixes issue with fatal error/out of range.
Closes #139

Checklist

• run npm run test && npm run benchmark --if-present
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[zekth]

Nice! Can we run benchmarks on this branch vs main?

[wooffie]

Nice! Can we run benchmarks on this branch vs main?

Actually no changes across this branch/main cause benchmark don't use enought long url to see it, and function not used across all library calls.

You can add benchmark suite:

suite.add('fast-uri: serialize uri', function () {
  fasturi.serialize({
    scheme: 'uri',
    userinfo: 'foo:bar',
    host: 'example.com',
    port: 1,
    path: './a/./b/c/../.././d/../e/f/.././/',
    query: 'query',
    fragment: 'fragment'
  })
})

Results:
main branch - fast-uri: serialize uri x 345,155 ops/sec ±3.12% (92 runs sampled)
this branch - fast-uri: serialize uri x 843,644 ops/sec ±2.90% (87 runs sampled)
urijs - urijs: serialize uri x 239,765 ops/sec ±0.57% (98 runs sampled)

[Uzlopak]

@zekth

Details

function removeDotSegments (path) {
  let input = path
  const output = []
  let nextSlash = -1
  let len = 0

  while (len = input.length) {
    if (len === 1) {
      if (input === '.') {
        break
      } else if (input === '/') {
        output.push('/')
        break
      } else {
        output.push(input)
        break
      }
    } else if (len === 2) {
      if (input[0] === '.') {
        if (input[1] === '.') {
          break
        } else if (input[1] === '/') {
          input = input.slice(2)
          continue
        }
      } else if (input[0] === '/') {
        if (input[1] === '.' || input[1] === '/') {
          output.push('/')
          break
        }
      }
    } else if (len === 3) {
      if (input === '/..') {
        if (output.length !== 0) {
          output.pop()
        }
        output.push('/')
        break
      }
    }
    if (input[0] === '.') {
      if (input[1] === '.') {
        if (input[2] === '/') {
          input = input.slice(3)
        }
      } else if (input[1] === '/') {
        input = input.slice(2)
      }
    } else if (input[0] === '/') {
      if (input[1] === '.') {
        if (input[2] === '/') {
          input = input.slice(2)
          continue
        } else if (input[2] === '.') {
          if (input[3] === '/') {
            input = input.slice(3)
            if (output.length !== 0) {
              output.pop()
            }
            continue
          }
        }
      }
    }

    // Rule 2E: Move normal path segment to output
    if ((nextSlash = input.indexOf('/', 1)) === -1) {
      output.push(input)
      break
    } else {
      output.push(input.slice(0, nextSlash))
      input = input.slice(nextSlash)
    }
  }

  return output.join('')
}

this pr:

aras@aras-HP-ZBook-15-G3:~/workspace/fast-uri$ node benchmark.js 
fast-uri: serialize uri x 559,853 ops/sec ±1.13% (91 runs sampled)

mine:

aras@aras-HP-ZBook-15-G3:~/workspace/fast-uri$ node benchmark.js 
fast-uri: serialize uri x 653,915 ops/sec ±1.55% (93 runs sampled)

[Fdawgs]

Needs a supporting unit test for the bug raised in #139

[gurgunday]

@zekth

this pr:

aras@aras-HP-ZBook-15-G3:~/workspace/fast-uri$ node benchmark.js 
fast-uri: serialize uri x 559,853 ops/sec ±1.13% (91 runs sampled)

mine:

aras@aras-HP-ZBook-15-G3:~/workspace/fast-uri$ node benchmark.js 
fast-uri: serialize uri x 653,915 ops/sec ±1.55% (93 runs sampled)

@wooffie if you can confirm these results, I'd suggest applying @Uzlopak's suggestion. I understand it might look a little worse but performance is a key part of the Fastify ecosystem, and besides I'm used to seeing code like this in URI specs

Thanks again!

[gurgunday]

lgtm

[Uzlopak]

@wooffie

Can you please add your benchmark to the suite?

Then we can make a final comparison between main and pr ;)

[wooffie]

My results:

fast-uri: serialize uri x 1,349,182 ops/sec ±6.04% (78 runs sampled)
urijs: serialize uri x 385,399 ops/sec ±2.95% (81 runs sampled)
fast-uri: serialize long uri with dots x 921,619 ops/sec ±3.07% (92 runs sampled)
urijs: serialize long uri with dots x 215,996 ops/sec ±0.49% (96 runs sampled

urijs has more complexity to work with dots.
Default test shows that urijs slower by 71%
Long test show 76%
Also if add more and more dots ('./a/./b/c/../.././d/../e/f/.././/a/3/24/bds/../../../2/../b/c//') we can get 80% and this difference will grow

[Uzlopak]

@wooffie

Well, it is more like, not that the changes resulted in a regression in other cases.

[Uzlopak]

I dont see any regression.

main:

fast-uri: parse domain x 1,118,155 ops/sec ±2.26% (90 runs sampled)
urijs: parse domain x 332,608 ops/sec ±0.70% (90 runs sampled)
WHATWG URL: parse domain x 2,862,487 ops/sec ±0.61% (94 runs sampled)
fast-uri: parse IPv4 x 1,602,475 ops/sec ±0.94% (92 runs sampled)
urijs: parse IPv4 x 287,487 ops/sec ±0.79% (94 runs sampled)
fast-uri: parse IPv6 x 860,477 ops/sec ±0.51% (94 runs sampled)
urijs: parse IPv6 x 222,171 ops/sec ±1.23% (94 runs sampled)
fast-uri: parse URN x 1,737,473 ops/sec ±0.51% (95 runs sampled)
urijs: parse URN x 859,554 ops/sec ±0.49% (93 runs sampled)
WHATWG URL: parse URN x 3,475,550 ops/sec ±0.60% (91 runs sampled)
fast-uri: parse URN uuid x 1,074,608 ops/sec ±0.99% (95 runs sampled)
urijs: parse URN uuid x 616,849 ops/sec ±0.59% (94 runs sampled)
fast-uri: serialize uri x 949,836 ops/sec ±1.49% (94 runs sampled)
urijs: serialize uri x 264,787 ops/sec ±0.47% (95 runs sampled)
fast-uri: serialize long uri with dots x 218,870 ops/sec ±0.66% (95 runs sampled)
urijs: serialize long uri with dots x 132,720 ops/sec ±0.72% (94 runs sampled)
fast-uri: serialize IPv6 x 409,919 ops/sec ±2.59% (91 runs sampled)
urijs: serialize IPv6 x 193,344 ops/sec ±0.47% (96 runs sampled)
fast-uri: serialize ws x 1,002,939 ops/sec ±0.65% (95 runs sampled)
urijs: serialize ws x 263,285 ops/sec ±0.56% (95 runs sampled)
fast-uri: resolve x 303,951 ops/sec ±0.75% (91 runs sampled)
urijs: resolve x 159,221 ops/sec ±2.24% (92 runs sampled)

PR:

fast-uri: parse domain x 1,120,644 ops/sec ±1.12% (90 runs sampled)
urijs: parse domain x 324,418 ops/sec ±1.68% (94 runs sampled)
WHATWG URL: parse domain x 2,781,028 ops/sec ±1.01% (93 runs sampled)
fast-uri: parse IPv4 x 1,574,421 ops/sec ±0.92% (95 runs sampled)
urijs: parse IPv4 x 270,356 ops/sec ±1.00% (93 runs sampled)
fast-uri: parse IPv6 x 829,637 ops/sec ±0.78% (93 runs sampled)
urijs: parse IPv6 x 214,412 ops/sec ±0.77% (95 runs sampled)
fast-uri: parse URN x 1,694,760 ops/sec ±0.90% (92 runs sampled)
urijs: parse URN x 844,838 ops/sec ±0.72% (97 runs sampled)
WHATWG URL: parse URN x 3,428,778 ops/sec ±0.59% (93 runs sampled)
fast-uri: parse URN uuid x 1,080,123 ops/sec ±0.62% (95 runs sampled)
urijs: parse URN uuid x 614,282 ops/sec ±0.63% (94 runs sampled)
fast-uri: serialize uri x 1,137,299 ops/sec ±0.78% (95 runs sampled)
urijs: serialize uri x 260,947 ops/sec ±0.55% (95 runs sampled)
fast-uri: serialize long uri with dots x 634,818 ops/sec ±0.69% (95 runs sampled)
urijs: serialize long uri with dots x 126,910 ops/sec ±0.63% (95 runs sampled)
fast-uri: serialize IPv6 x 427,203 ops/sec ±0.75% (96 runs sampled)
urijs: serialize IPv6 x 192,218 ops/sec ±1.24% (94 runs sampled)
fast-uri: serialize ws x 1,218,515 ops/sec ±1.29% (95 runs sampled)
urijs: serialize ws x 246,424 ops/sec ±1.12% (95 runs sampled)
fast-uri: resolve x 473,679 ops/sec ±0.92% (94 runs sampled)
urijs: resolve x 161,916 ops/sec ±0.58% (94 runs sampled)

[Uzlopak]

But there is one question left: Is it still everything according to the RFC? :D.

[Uzlopak]

I wrote some more unit tests to ensure that the rfc is met...well it wasnt. I missed two continue operators.

I pushed into this PR.

[gurgunday]

lgtm

[zekth]

Excellent