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

Use WebCrypto API #143

Merged
merged 25 commits into from
May 28, 2024
Merged

Use WebCrypto API #143

merged 25 commits into from
May 28, 2024

Conversation

7heMech
Copy link
Contributor

@7heMech 7heMech commented May 6, 2024

I've modified:

  • the code to use webcrypto api
  • the tests to work properly with promises
  • README.md to reflect that library is now async

@7heMech
Copy link
Contributor Author

7heMech commented May 6, 2024

Not sure if it works on node <20 🥶 (because of the way crypto api is referenced, might need to change that)

@7heMech
Copy link
Contributor Author

7heMech commented May 6, 2024

Btw, I'm already using this in production 😂

@7heMech
Copy link
Contributor Author

7heMech commented May 7, 2024

Will make one more commit.

@bellstrand
Copy link
Owner

I'll have a closer look at the code soon, but before that we'll need to address the NodeJS 18 error (I'm okay with dropping support for Node 16 since it went out of maintenance half a year ago, but Node 18 will need to be supported for another year until it reaches its end of life.

I've also tried running some simple performance tests with the following code (you have to remove the jest clock)

test.only.each([10, 100, 1000, 10000, 50000])("performance, %p", async (index) => {
	const now = Date.now()
	let timestamp = 0
	for (let i = 0; i < index; i++) {
		await TOTP.generate("JBSWY3DPEHPK3PXP", { timestamp: timestamp++ })
	}
	console.log(`${(Date.now() - now) / 1000}s`)
})

And currently this branch takes approximately double the time to execute a TOTP.generate compared to the main branch (I'm running this on node v21.7.2 when trying it out).

I'll gladly help to unravel the performance issues in a few days!

@7heMech
Copy link
Contributor Author

7heMech commented May 7, 2024

I'll have a closer look at the code soon, but before that we'll need to address the NodeJS 18 error (I'm okay with dropping support for Node 16 since it went out of maintenance half a year ago, but Node 18 will need to be supported for another year until it reaches its end of life.

I've also tried running some simple performance tests with the following code (you have to remove the jest clock)

test.only.each([10, 100, 1000, 10000, 50000])("performance, %p", async (index) => {
	const now = Date.now()
	let timestamp = 0
	for (let i = 0; i < index; i++) {
		await TOTP.generate("JBSWY3DPEHPK3PXP", { timestamp: timestamp++ })
	}
	console.log(`${(Date.now() - now) / 1000}s`)
})

And currently this branch takes approximately double the time to execute a TOTP.generate compared to the main branch (I'm running this on node v21.7.2 when trying it out).

I'll gladly help to unravel the performance issues in a few days!

Hey, there,
The nodejs <20 support is just a reference issue, it won't be hard to fix.
I've tried to do some performance optimizations, but apparently it wasn't enough, maybe my implementation is not very good, I'll do some debugging as well.

@bellstrand
Copy link
Owner

I can help you with trying to optimize it, but I'm out travling for another ~10 days, so it will take a little while before being able to do much ;)

@7heMech
Copy link
Contributor Author

7heMech commented May 7, 2024

I can help you with trying to optimize it, but I'm out travling for another ~10 days, so it will take a little while before being able to do much ;)

Much appreciated, I'll see what I can do meanwhile.

@7heMech
Copy link
Contributor Author

7heMech commented May 7, 2024

@bellstrand I've got a question, so I had in my base32 to buffer function a piece of code which calculates a lookup object with charCode to Index for base32 alphabet, so in order to optimize (even a little) I did this:

/**
 * A precalculated mapping from base32 character codes to their corresponding index values for performance optimization.
 * This mapping is used in the base32ToBuffer method to convert base32 encoded strings to their binary representation.
 */
private static readonly base32: { [key: string]: number } = { "50": 26, "51": 27, "52": 28, "53": 29, "54": 30, "55": 31, "65": 0, "66": 1, "67": 2, "68": 3, "69": 4, "70": 5, "71": 6, "72": 7, "73": 8, "74": 9, "75": 10, "76": 11, "77": 12, "78": 13, "79": 14, "80": 15, "81": 16, "82": 17, "83": 18, "84": 19, "85": 20, "86": 21, "87": 22, "88": 23, "89": 24, "90": 25 }

does this look ok?
It's messing with prettier wanting to format it, maybe I should move it to a separate file?

@7heMech
Copy link
Contributor Author

7heMech commented May 7, 2024

Tested on first versions of node 16 and 18, it works!

@bellstrand
Copy link
Owner

@bellstrand I've got a question, so I had in my base32 to buffer function a piece of code which calculates a lookup object with charCode to Index for base32 alphabet, so in order to optimize (even a little) I did this:

/**
 * A precalculated mapping from base32 character codes to their corresponding index values for performance optimization.
 * This mapping is used in the base32ToBuffer method to convert base32 encoded strings to their binary representation.
 */
private static readonly base32: { [key: string]: number } = { "50": 26, "51": 27, "52": 28, "53": 29, "54": 30, "55": 31, "65": 0, "66": 1, "67": 2, "68": 3, "69": 4, "70": 5, "71": 6, "72": 7, "73": 8, "74": 9, "75": 10, "76": 11, "77": 12, "78": 13, "79": 14, "80": 15, "81": 16, "82": 17, "83": 18, "84": 19, "85": 20, "86": 21, "87": 22, "88": 23, "89": 24, "90": 25 }

does this look ok? It's messing with prettier wanting to format it, maybe I should move it to a separate file?

We can look at moving it to a different file, but might need to do something with the build script to bundle it in one file for that!

@7heMech
Copy link
Contributor Author

7heMech commented May 7, 2024

Look at bun branch test speed lol

test.lol.mp4

@7heMech
Copy link
Contributor Author

7heMech commented May 7, 2024

@bellstrand I've got a question, so I had in my base32 to buffer function a piece of code which calculates a lookup object with charCode to Index for base32 alphabet, so in order to optimize (even a little) I did this:

/**
 * A precalculated mapping from base32 character codes to their corresponding index values for performance optimization.
 * This mapping is used in the base32ToBuffer method to convert base32 encoded strings to their binary representation.
 */
private static readonly base32: { [key: string]: number } = { "50": 26, "51": 27, "52": 28, "53": 29, "54": 30, "55": 31, "65": 0, "66": 1, "67": 2, "68": 3, "69": 4, "70": 5, "71": 6, "72": 7, "73": 8, "74": 9, "75": 10, "76": 11, "77": 12, "78": 13, "79": 14, "80": 15, "81": 16, "82": 17, "83": 18, "84": 19, "85": 20, "86": 21, "87": 22, "88": 23, "89": 24, "90": 25 }

does this look ok? It's messing with prettier wanting to format it, maybe I should move it to a separate file?

We can look at moving it to a different file, but might need to do something with the build script to bundle it in one file for that!

Oh, fair enough.

@7heMech
Copy link
Contributor Author

7heMech commented May 7, 2024

@bellstrand I think I got it, since I await each promise it might be slower, but if I use promise.all?

@bellstrand
Copy link
Owner

@bellstrand I think I got it, since I await each promise it might be slower, but if I use promise.all?

Well, you use the result of the first promise in the second await, so sadly that won't work!

@7heMech
Copy link
Contributor Author

7heMech commented May 7, 2024

If your tests do the same (use promise.all to run all tests "at the same time") instead of each awaited, it will be way faster.
Edit: this is with promise.all of current code
image

@7heMech
Copy link
Contributor Author

7heMech commented May 7, 2024

I got some perf calc going
image
I guess it really depends on the runtime, this is in bun land
image

@7heMech
Copy link
Contributor Author

7heMech commented May 7, 2024

Well, at least we got isomorphic package now

@7heMech
Copy link
Contributor Author

7heMech commented May 8, 2024

@bellstrand I read some stuff on the topic and it seems like because of V8 promises, the webcrypto API is actually slower than third-party libraries for small amounts of data, not sure if it's still relevant, but this might be why.

@7heMech
Copy link
Contributor Author

7heMech commented May 8, 2024

@bellstrand I can't seem to be able to get jest to think I have tests for everything

@bellstrand
Copy link
Owner

bellstrand commented May 8, 2024

It went through the coverage tests on the last run thou! ;)
But as I said, I'm away for ~10 days, so will merge and release a new major version when I get back (since we're doing a breaking change) with moving from sync => async.

@7heMech
Copy link
Contributor Author

7heMech commented May 8, 2024

@bellstrand sorry for bothering your travel 😅
I'll leave it at this state till you're available again, today I implemented minification

@7heMech
Copy link
Contributor Author

7heMech commented May 19, 2024

I'll run some benchmarks with mitata comparing the libs to check performance.
Edit: yeah, it's a bit slower lol, but at those numbers I'm not sure if it even matters:
image

@7heMech
Copy link
Contributor Author

7heMech commented May 25, 2024

Okay, so I don't really think I can do anything more about this I'd say this is finished state, but do tell me if you want me to change something (or everything 😅)

@7heMech
Copy link
Contributor Author

7heMech commented May 25, 2024

@bellstrand not sure if you get notifications without ping.

@bellstrand
Copy link
Owner

@7heMech Sorry for being slow, I stayed abroad for another week.
I think everything looks great, let's just resolve the yarn.lock conflict and I'll merge everything! :)

@7heMech
Copy link
Contributor Author

7heMech commented May 28, 2024

@bellstrand resolved

@bellstrand bellstrand merged commit d64d40e into bellstrand:master May 28, 2024
5 checks passed
@7heMech 7heMech deleted the webcrypto branch May 31, 2024 07:21
@7heMech
Copy link
Contributor Author

7heMech commented May 31, 2024

@bellstrand I'll do some more changes soon (maybe ASCII encoding key and fix some TS stuff) or at least suggest them as a pr.

@7heMech 7heMech restored the webcrypto branch May 31, 2024 16:32
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.

2 participants