Skip to content

Conversation

@sivakumarsc
Copy link

@sivakumarsc sivakumarsc commented Aug 27, 2025

Which problem is this PR solving?

While analysing our application CPU profile, we found parsePairKeyValue from @opentelemetry/core is one of the hot function.
CPU_Profile

Fixes # (issue)

Short description of the changes

Refactor the parsePairKeyValue function to simplify the computations

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

A Benchmark test to compare the Older and newer implementation

Benchmark

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@sivakumarsc sivakumarsc requested a review from a team as a code owner August 27, 2025 16:36
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 27, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: maryliag / name: Marylia Gutierrez (86e7cb2)
  • ✅ login: sivakumarsc / name: Sivakumar (87ca28a)

@codecov
Copy link

codecov bot commented Sep 3, 2025

Codecov Report

❌ Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 95.21%. Comparing base (77ab165) to head (87ca28a).

Files with missing lines Patch % Lines
packages/opentelemetry-core/src/baggage/utils.ts 94.11% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5885      +/-   ##
==========================================
- Coverage   95.21%   95.21%   -0.01%     
==========================================
  Files         316      316              
  Lines        9389     9398       +9     
  Branches     2167     2171       +4     
==========================================
+ Hits         8940     8948       +8     
- Misses        449      450       +1     
Files with missing lines Coverage Δ
packages/opentelemetry-core/src/baggage/utils.ts 97.72% <94.11%> (-2.28%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@trentm
Copy link
Contributor

trentm commented Sep 3, 2025

I haven't dug into why yet, but the new impl shows to be the same time or slower for me:

[15:39:10 trentm@peach:~/tm/opentelemetry-js4/packages/opentelemetry-core (git:refactor-parse-pair-key-value)]
% npm run test:bench

> @opentelemetry/[email protected] test:bench
> node test/performance/benchmark/index.js

parsePairKeyValue simple (old) x 5,054,037 ops/sec ±0.67% (93 runs sampled)
parsePairKeyValue simple (new) x 5,039,987 ops/sec ±0.68% (95 runs sampled)
parsePairKeyValue with metadata (old) x 4,525,204 ops/sec ±0.69% (92 runs sampled)
parsePairKeyValue with metadata (new) x 4,346,512 ops/sec ±0.58% (92 runs sampled)
parsePairKeyValue URI encoded (old) x 4,430,196 ops/sec ±0.62% (96 runs sampled)
parsePairKeyValue URI encoded (new) x 4,435,236 ops/sec ±0.61% (96 runs sampled)
parsePairKeyValue complex (old) x 3,606,884 ops/sec ±0.71% (93 runs sampled)
parsePairKeyValue complex (new) x 3,496,362 ops/sec ±0.70% (97 runs sampled)
[15:40:01 trentm@peach:~/tm/opentelemetry-js4/packages/opentelemetry-core (git:refactor-parse-pair-key-value)]
% node --version
v20.19.1

Similar on Node.js 24:

% node --version
v24.6.0
[15:41:22 trentm@peach:~/tm/opentelemetry-js4/packages/opentelemetry-core (n:24 git:refactor-parse-pair-key-value)]
% npm run test:bench

> @opentelemetry/[email protected] test:bench
> node test/performance/benchmark/index.js

parsePairKeyValue simple (old) x 5,442,733 ops/sec ±0.74% (95 runs sampled)
parsePairKeyValue simple (new) x 5,459,702 ops/sec ±0.85% (95 runs sampled)
parsePairKeyValue with metadata (old) x 4,800,913 ops/sec ±0.70% (93 runs sampled)
parsePairKeyValue with metadata (new) x 4,682,423 ops/sec ±0.76% (94 runs sampled)
parsePairKeyValue URI encoded (old) x 4,505,672 ops/sec ±0.66% (94 runs sampled)
parsePairKeyValue URI encoded (new) x 4,530,546 ops/sec ±0.95% (89 runs sampled)
parsePairKeyValue complex (old) x 3,611,070 ops/sec ±0.98% (95 runs sampled)
parsePairKeyValue complex (new) x 3,680,629 ops/sec ±0.74% (96 runs sampled)

FWIW: This is on macos 15.6.1, arm64.

@sivakumarsc
Copy link
Author

npm run test:bench

Not sure, why you are seeing different numbers with the same config.

My machine - MacOs - 15.6.1, Mac M3 pro

Node 20.11.1

Screenshot 2025-09-05 at 11 31 21 AM

Node 24.6.0
Screenshot 2025-09-05 at 11 33 53 AM

@david-luna
Copy link
Contributor

my results are

for node v20.19.1

npm run test:bench

> @opentelemetry/[email protected] test:bench
> node test/performance/benchmark/index.js

parsePairKeyValue simple (old) x 4,692,843 ops/sec ±0.41% (94 runs sampled)
parsePairKeyValue simple (new) x 6,267,494 ops/sec ±0.32% (97 runs sampled)
parsePairKeyValue with metadata (old) x 4,180,132 ops/sec ±0.32% (97 runs sampled)
parsePairKeyValue with metadata (new) x 5,656,792 ops/sec ±0.33% (100 runs sampled)
parsePairKeyValue URI encoded (old) x 4,116,159 ops/sec ±1.15% (98 runs sampled)
parsePairKeyValue URI encoded (new) x 5,144,204 ops/sec ±0.55% (99 runs sampled)
parsePairKeyValue complex (old) x 3,226,162 ops/sec ±0.28% (99 runs sampled)
parsePairKeyValue complex (new) x 4,714,852 ops/sec ±0.27% (99 runs sampled)

node --version
v20.19.1

for node v24.0.2

npm run test:bench

> @opentelemetry/[email protected] test:bench
> node test/performance/benchmark/index.js

parsePairKeyValue simple (old) x 4,786,133 ops/sec ±0.34% (99 runs sampled)
parsePairKeyValue simple (new) x 5,645,059 ops/sec ±0.29% (97 runs sampled)
parsePairKeyValue with metadata (old) x 4,332,720 ops/sec ±0.35% (100 runs sampled)
parsePairKeyValue with metadata (new) x 4,920,477 ops/sec ±0.34% (96 runs sampled)
parsePairKeyValue URI encoded (old) x 4,149,403 ops/sec ±0.30% (97 runs sampled)
parsePairKeyValue URI encoded (new) x 4,647,395 ops/sec ±0.42% (100 runs sampled)
parsePairKeyValue complex (old) x 3,416,122 ops/sec ±0.33% (99 runs sampled)
parsePairKeyValue complex (new) x 4,128,169 ops/sec ±0.33% (98 runs sampled)

node --version
v24.0.2

This is on MacOS 15.7.1 with M2 chip. Could it be related to the chip?

@sivakumarsc
Copy link
Author

sivakumarsc commented Oct 16, 2025

@david-luna Not sure if this due to chip. But for me refactored function always shows better runtime.

@pichlermarc pichlermarc self-assigned this Nov 12, 2025
@maryliag
Copy link
Contributor

maryliag commented Nov 12, 2025

my results

opentelemetry-js/packages/opentelemetry-core (8a81b53) [$] is 📦 v2.2.0 via  v22.20.0 
❯ npm run test:bench

> @opentelemetry/[email protected] test:bench
> node test/performance/benchmark/index.js

parsePairKeyValue simple (old) x 4,127,922 ops/sec ±0.46% (97 runs sampled)
parsePairKeyValue simple (new) x 4,072,673 ops/sec ±3.54% (99 runs sampled)
parsePairKeyValue with metadata (old) x 3,331,769 ops/sec ±9.79% (92 runs sampled)
parsePairKeyValue with metadata (new) x 3,552,481 ops/sec ±0.75% (95 runs sampled)
parsePairKeyValue URI encoded (old) x 3,663,071 ops/sec ±1.20% (97 runs sampled)
parsePairKeyValue URI encoded (new) x 3,615,141 ops/sec ±1.33% (97 runs sampled)
parsePairKeyValue complex (old) x 3,137,890 ops/sec ±1.20% (97 runs sampled)
parsePairKeyValue complex (new) x 3,100,433 ops/sec ±0.31% (95 runs sampled)

Using MacOs 15.6.1 with M3 chip

@JamieDanielson
Copy link
Member

my results

opentelemetry-js/packages/opentelemetry-core (8a81b53) [$] is 📦 v2.2.0 via  v22.20.0 

@maryliag what machine are you using? (and what chip 😅 )

@maryliag
Copy link
Contributor

updated my comment with the details

@JamieDanielson
Copy link
Member

My results with Apple M2 Max MacOS 15.6.1

v20.19.1

> @opentelemetry/[email protected] test:bench
> node test/performance/benchmark/index.js

parsePairKeyValue simple (old) x 4,511,325 ops/sec ±0.20% (98 runs sampled)
parsePairKeyValue simple (new) x 5,967,220 ops/sec ±0.25% (97 runs sampled)
parsePairKeyValue with metadata (old) x 3,996,706 ops/sec ±0.24% (96 runs sampled)
parsePairKeyValue with metadata (new) x 5,393,981 ops/sec ±0.96% (96 runs sampled)
parsePairKeyValue URI encoded (old) x 3,972,514 ops/sec ±0.25% (98 runs sampled)
parsePairKeyValue URI encoded (new) x 5,015,165 ops/sec ±0.17% (96 runs sampled)
parsePairKeyValue complex (old) x 3,134,005 ops/sec ±0.22% (96 runs sampled)
parsePairKeyValue complex (new) x 4,542,942 ops/sec ±0.35% (95 runs sampled)

node --version
v20.19.1

v24.0.2

> @opentelemetry/[email protected] test:bench
> node test/performance/benchmark/index.js

parsePairKeyValue simple (old) x 4,715,809 ops/sec ±0.32% (98 runs sampled)
parsePairKeyValue simple (new) x 5,385,761 ops/sec ±0.31% (97 runs sampled)
parsePairKeyValue with metadata (old) x 4,117,043 ops/sec ±1.51% (95 runs sampled)
parsePairKeyValue with metadata (new) x 4,801,934 ops/sec ±0.27% (97 runs sampled)
parsePairKeyValue URI encoded (old) x 3,974,324 ops/sec ±0.22% (98 runs sampled)
parsePairKeyValue URI encoded (new) x 4,450,870 ops/sec ±0.19% (97 runs sampled)
parsePairKeyValue complex (old) x 3,299,793 ops/sec ±0.27% (96 runs sampled)
parsePairKeyValue complex (new) x 4,002,586 ops/sec ±0.17% (99 runs sampled)

node --version
v24.0.2

@pichlermarc
Copy link
Member

@maryliag @trentm - silly question: did you npm run compile before running the benchmark? 🤔

Benchmark tests do not use ts-node to avoid the overhead that's introduced by it. My results are pretty much in line with what @david-luna and @JamieDanielson are seeing. However, without compiling first I'm comparing the old implementation with itself, which looks somewhat similar to the results you're getting. 🤔

@maryliag
Copy link
Contributor

maryliag commented Nov 17, 2025

Compiling made a difference :D

opentelemetry-js/packages/opentelemetry-core (8a81b53) [$] is 📦 v2.2.0 via  v22.20.0 
❯ npm run compile

> @opentelemetry/[email protected] compile
> tsc --build tsconfig.json tsconfig.esm.json tsconfig.esnext.json


opentelemetry-js/packages/opentelemetry-core (8a81b53) [$] is 📦 v2.2.0 via  v22.20.0 
❯ npm run test:bench

> @opentelemetry/[email protected] test:bench
> node test/performance/benchmark/index.js

parsePairKeyValue simple (old) x 4,052,323 ops/sec ±1.21% (93 runs sampled)
parsePairKeyValue simple (new) x 4,527,738 ops/sec ±0.73% (96 runs sampled)
parsePairKeyValue with metadata (old) x 3,759,234 ops/sec ±0.43% (95 runs sampled)
parsePairKeyValue with metadata (new) x 4,163,401 ops/sec ±0.44% (96 runs sampled)
parsePairKeyValue URI encoded (old) x 3,714,385 ops/sec ±0.69% (95 runs sampled)
parsePairKeyValue URI encoded (new) x 4,024,217 ops/sec ±0.33% (96 runs sampled)
parsePairKeyValue complex (old) x 3,194,446 ops/sec ±0.78% (97 runs sampled)
parsePairKeyValue complex (new) x 3,741,602 ops/sec ±0.33% (97 runs sampled)

@sivakumarsc
Copy link
Author

Thanks @pichlermarc @maryliag. @trentm Could you also please try once with compiled lib?

Copy link
Contributor

@trentm trentm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

silly question: did you npm run compile before running the benchmark? 🤔

Oh man. 🤦
I now get real results. Sorry about causing delay because of this.

Significant speed improvement for Node v20 (macos/arm64):

% npm run test:bench

> @opentelemetry/[email protected] test:bench
> node test/performance/benchmark/index.js

parsePairKeyValue simple (old) x 5,428,750 ops/sec ±0.52% (97 runs sampled)
parsePairKeyValue simple (new) x 7,139,674 ops/sec ±0.38% (95 runs sampled)
parsePairKeyValue with metadata (old) x 4,851,471 ops/sec ±0.31% (96 runs sampled)
parsePairKeyValue with metadata (new) x 6,324,504 ops/sec ±0.46% (94 runs sampled)
parsePairKeyValue URI encoded (old) x 4,730,705 ops/sec ±1.02% (98 runs sampled)
parsePairKeyValue URI encoded (new) x 5,935,547 ops/sec ±0.38% (100 runs sampled)
parsePairKeyValue complex (old) x 3,849,635 ops/sec ±0.28% (95 runs sampled)
parsePairKeyValue complex (new) x 5,391,519 ops/sec ±0.28% (99 runs sampled)

% node --version
v20.19.1

Slightly less significant, but still good improvement for Node.js v24, FWIW:

% node --version
v24.6.0

% npm run test:bench

> @opentelemetry/[email protected] test:bench
> node test/performance/benchmark/index.js

parsePairKeyValue simple (old) x 5,614,083 ops/sec ±0.66% (91 runs sampled)
parsePairKeyValue simple (new) x 6,470,560 ops/sec ±0.37% (96 runs sampled)
parsePairKeyValue with metadata (old) x 5,127,297 ops/sec ±0.35% (98 runs sampled)
parsePairKeyValue with metadata (new) x 5,803,653 ops/sec ±0.51% (97 runs sampled)
parsePairKeyValue URI encoded (old) x 4,749,873 ops/sec ±0.31% (98 runs sampled)
parsePairKeyValue URI encoded (new) x 5,259,616 ops/sec ±0.37% (98 runs sampled)
parsePairKeyValue complex (old) x 3,916,728 ops/sec ±0.35% (98 runs sampled)
parsePairKeyValue complex (new) x 4,824,857 ops/sec ±0.38% (96 runs sampled)

Q: Do we need/want to commit the microbenchmark? Its use is entirely for this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants