-
Notifications
You must be signed in to change notification settings - Fork 996
perf(sdk-trace-base): use Uint8Array for browser RandomIdGenerator #6209
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(sdk-trace-base): use Uint8Array for browser RandomIdGenerator #6209
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6209 +/- ##
==========================================
+ Coverage 95.46% 95.58% +0.11%
==========================================
Files 317 314 -3
Lines 9602 9590 -12
Branches 2221 2221
==========================================
Hits 9167 9167
+ Misses 435 423 -12 🚀 New features to boost your workflow:
|
| for (let i = 0; i < buf.length; i++) { | ||
| buf[i] = (Math.random() * 256) >>> 0; | ||
| } | ||
| // Ensure non-zero | ||
| for (let i = 0; i < buf.length; i++) { | ||
| if (buf[i] > 0) return; | ||
| } | ||
| buf[buf.length - 1] = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: did you consider using crypto.getRandomValues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without running a test i can't say for sure but I'd say its very unlikely the crypto module is fast enough. The "cryptographically strong" requirement of that module trades off speed and its pretty significant in a lot of cases. We only have need for statistical randomness (uniform distribution) here. The distinction is that it might be possible to guess which trace id is generated in some cases (through things like timing attacks), but that's fine in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I did try crypto: 10x slower than the original 🫠
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, apparently it's slower than your solution... this was unexpected 😄 https://jsben.ch/ZR73M
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were actually using crypto module in the past and removed it for performance reasons https://github.com/open-telemetry/opentelemetry-js/pull/1349/changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just added a benchmark test.
| buf[i] = (Math.random() * 256) >>> 0; | ||
| } | ||
| // Ensure non-zero | ||
| for (let i = 0; i < buf.length; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: as a fallback a '1' is written in the last position to ensure the buf does not contain all zeroes. Maybe accessing a random position of the array and setting it to '1' if not already set would allow is to skip a second iteration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe accessing a random position of the array and setting it to '1' if not already set would allow is to skip a second iteration.
I'm not a mathematician but I'd worry a bit that that biases the distribution away from values with 0s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me neither but what you said makes a lot of sense. The I guess a flag in the loop that fills the buf would be enough
Something like
let allZero = true;
for (let i = 0; i < buf.length; i++) {
buf[i] = (Math.random() * 256) >>> 0;
allZero = allZero && buf[i] === 0;
}
if (allZero) {
buf[buf.length - 1] = 1;
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@david-luna the allZero check in the loop is about 20% slower than the proposed function.
trentm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with a Q about dropping the benchmark file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we keep this file? This is a benchmark running node using browser code. I think quoting some benchmark code (or perhaps a https://jsben.ch or similar link) showing browser numbers in the PR discussion would be good, but not sure of the value of having the benchmark file persisting in the repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point. Let me try to add a browser bench test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trentm I moved some files around so the platform chosen in package.json is used for common tests, including the new benchmark test for the randomIdGenerator. It expanded the scope quite a bit but now reports numbers for node and browser. I updated the description above with node, browser (main), and browser (this branch) results.
If this is too many changes for this PR I can back them out and make a new branch for the test folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trentm I added browser benchmark tests that do not write to disk. I just saw @pichlermarc's note about being judicious with the amount of benchmarks published.
I defer to you whether or not they should be published (in a follow-up)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks. Sounds good to me. My main concern had been about the (lack of) value of having those metrics included in the published benchmarks.
Marc had mentioned that he has a PR coming at some point to reduce the set of metrics that will be included in the published set (by just changing the benchmark to not output results to the benchmark.txt file that is slurped up by the benchmark.yml CI, I believe).
- move test/common/platform/RandomIdGenerator.test.ts to test/common/ - move test/common/Sampler.test.ts to test/common/sampler/ - add benchmarks for RandomIdGenerator, Span, and BatchSpanProcessor - benchmarks run on both Node and browser platforms - add karma.bench.js config for browser benchmarks - add AMD stub to karma.webpack.js for Benchmark.js compatibility
be43a55 to
bb60e72
Compare
bb60e72 to
4c703e2
Compare
trentm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this LGTM. Impl is good. I didn't dig into the browser bench scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks. Sounds good to me. My main concern had been about the (lack of) value of having those metrics included in the published benchmarks.
Marc had mentioned that he has a PR coming at some point to reduce the set of metrics that will be included in the published set (by just changing the benchmark to not output results to the benchmark.txt file that is slurped up by the benchmark.yml CI, I believe).
Summary
RandomIdGeneratorby usingUint8Arraywith a byte-to-hex lookup table instead of character code generation withString.fromCharCodeBenchmark Results
generateTraceIdgenerateSpanIdTest plan
npm run test:bench) and browser (npm run test:bench:browser)Benchmark Infrastructure
Separate Node and browser benchmarks
Benchmarks are split into separate files to allow distinct test names for reporting:
test/node/*.bench.ts- Node benchmarks (e.g.,generateTraceId)test/browser/*.bench.ts- Browser benchmarks with "(browser)" suffix (e.g.,generateTraceId (browser))Output files
test/node/.benchmark-results.txt- Node benchmark resultsTest folder structure
test/node/- Node-only tests and benchmarkstest/browser/- Browser-only tests and benchmarks