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

feat: use cookie module again #194

Merged
merged 1 commit into from
Jul 14, 2022

Conversation

kurtextrem
Copy link
Contributor

@kurtextrem kurtextrem commented Jul 14, 2022

Previously, in PR #152 we removed the cookie dep., since the repo was stale.
The cookie module seems to be active again, and now actually the current implementation was outdated and lacking a performance fix from ~4 months ago that drastically pushes perf.

Checklist

  • run npm run test and npm run benchmark -> (there is no benchmark script btw)
  • tests and/or benchmarks are included -> nothing to do
  • documentation is changed or added -> nothing to do
  • commit message and code follows the Developer's Certification of Origin
    and the Code of conduct

Previously, in PR fastify#152 we removed the `cookie` dep., since the repo was stale.
The cookie module seems to be active again, and now actually the current implementation was outdated and lacking a performance fix from ~4 months ago.
@Uzlopak
Copy link
Contributor

Uzlopak commented Jul 14, 2022

It seems like our implementation is still slightly faster?

@Uzlopak
Copy link
Contributor

Uzlopak commented Jul 14, 2022

Well I guess the perf difference is neglectable

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

@mcollina mcollina merged commit f417694 into fastify:master Jul 14, 2022
@mcollina mcollina mentioned this pull request Jul 14, 2022
4 tasks
@kurtextrem
Copy link
Contributor Author

It seems like our implementation is still slightly faster?

Maybe this presents an opportunity to bring changes to upstream cookie?

I see differences for serialize (cookie):

if (opt.expires) {	
    var expires = opt.expires	
    if (!isDate(expires) || isNaN(expires.valueOf())) {	
      throw new TypeError('option expires is invalid');	
    }	
    str += '; Expires=' + expires.toUTCString()	
}

vs fastify-cookie:

if (opt.expires) {
    if (typeof opt.expires.toUTCString !== 'function') {
      throw new TypeError('option expires is invalid')
    }
    str += '; Expires=' + opt.expires.toUTCString()
}

seems like cookie has the safer implementation here. Fastify-cookie didn't include the Priority option (which is why using the upstream version makes more sense -> they take care of staying up2date with the latest cookie spec).
=> Stick with cookie implementation

However, coming back to parsing (fastify-cookie):

const key = str.substring(pos, eqIdx++).trim()
if (undefined === result[key]) {
      const val = (str.charCodeAt(eqIdx) === 0x22)
        ? str.substring(eqIdx + 1, terminatorPos - 1).trim()
        : str.substring(eqIdx, terminatorPos).trim()

      result[key] = (dec !== decode || val.indexOf('%') !== -1)
        ? tryDecode(val, dec)
        : val
}

vs cookie:

var key = str.slice(pos, eqIdx).trim()
if (undefined === result[key]) {	
      var val = str.slice(eqIdx + 1, endIdx).trim()	

      if (val.charCodeAt(0) === 0x22) {	
        val = val.slice(1, -1)	
      }	

      result[key] = tryDecode(val, dec);	
}

in the fastify-cookie implementation we only do 2 substring (slice) calls, in cookie we do 3 in the worst case.
in the fastify-cookie implementation we always run tryDecode also if a custom decoding function is provided, in cookie we don't (the indexOf('%') is done in tryDecode in the cookie implementation). Actually, .includes('%') should be faster than indexOf('%') !== -1 in modern Node?)
=> Create a PR with the fastify-cookie implementation?

and finally there is a bigger difference here:
image
and I don't really know what the best solution is. Looking at the function calls, it seems like fastify-cookie is more optimized here (no lastIndexOf call), but I'm not sure if both implementations have the same functionality?
(and fastify-cookie pulls the variable re-declarations inside the while loop outside of the block)
=> Unsure, let me know your thoughts

@Uzlopak
Copy link
Contributor

Uzlopak commented Jul 14, 2022

I had to smirk about this: they take care of staying up2date with the latest cookie spec

@kurtextrem
Copy link
Contributor Author

I had to smirk about this: they take care of staying up2date with the latest cookie spec

(I'm always trying to be positive about the future! 😄)

@kurtextrem
Copy link
Contributor Author

I've put up another PR: jshttp/cookie#144
I'm not convinced about removing the lastIndexOf call, in benchmarks I saw no difference, but then again I'm not really sure what exactly is happening there. I think the lastIndexOf call might save us more time for longer strings, than the terminatorPos + 1 (endIdx in cookie) does

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

3 participants