perf: optimize regex, remove uuid()#1044
Conversation
Chrome DevTools Protocol Tracing (Script: 106.76ms, Heap: 13.94MB)
Lighthouse (Script Eval: 49.98ms)
Platform Tests (vite-7 gzip: 47.60KB)vite-7 Platform Tests
vite-otel-latest Platform Tests
webpack-5 Platform Tests
|
There was a problem hiding this comment.
I'd be concerned about replacing the uuid library with our own implementation without more extensive testing around the values we're getting out being similarly random. Could you also speak to where the performance gains are relative to https://github.com/uuidjs/uuid/blob/main/src/v4.ts ?
There was a problem hiding this comment.
I benchmarked more thoroughly and the performance is negligible:
Browser Benchmark (Chromium)
| Implementation | ops/sec | ns/op | vs uuid |
|------------------------|---------|-------|---------|
| uuid library | 925K | 1,081 | 1.00x |
| Math.random | 8.7M | 115 | 9.42x |
| crypto.getRandomValues | 1.1M | 930 | 1.16x |
Absolute impact per 1,000 spans:
- uuid library: 1.1ms
- Math.random: 0.1ms
- crypto.getRandomValues: 0.9ms
What we do gain:
- Removing
uuidmeans one less third-party lib cryptois more secure than Math.random and slightly faster thanuuidnpm package
There was a problem hiding this comment.
do we know why this ends up faster than the uuid package? It is also using crypto.getRandomValues: https://github.com/uuidjs/uuid/blob/main/src/rng-browser.ts#L18
There was a problem hiding this comment.
Surprisingly it's the string manipulation actions to change case and add/remove hyphens.
There was a problem hiding this comment.
do we know why this ends up faster than the uuid package? It is also using crypto.getRandomValues: https://github.com/uuidjs/uuid/blob/main/src/rng-browser.ts#L18
| export const generateUUID = () => uuid().replace(/-/g, '').toUpperCase(); | ||
| // Pre-computed uppercase hex lookup - faster than toString(16) in browsers | ||
| const HEX: string[] = Array.from({ length: 256 }, (_, i) => | ||
| i.toString(16).padStart(2, '0').toUpperCase(), |
There was a problem hiding this comment.
would we need the i + 0x100 piece that is being done in https://github.com/uuidjs/uuid/blob/main/src/stringify.ts#L10 as well? Or is that handled by padStart?
There was a problem hiding this comment.
Correct, padStart gives the same 2 digit guarantee
What problem is this solving?
Reduces SDK CPU overhead by optimizing hot paths identified via performance profiling.
Short description of changes
.exec()withurl.includes('://')inisNetworkSpantype guardAdopt OTel byte-to-hex UUID algorithm (from opentelemetry-js#6209), removinguuiddependencycrypto.getRandomValues()instead ofuuidnpm packageTesting