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

Minimum viable example - benchmark issues #3146

Closed
wants to merge 3 commits into from

Conversation

jbertran
Copy link
Contributor

@jbertran jbertran commented May 12, 2023

What does this PR do?

This reproduces the benchmark issues seen in #2941

This example does 2 things:

  1. Introduce a few new modules (see below for require graph)
  2. Use plugin_manager's configure to perform a tiny operation at startup time.
            ┌─ plugin_manager
            │
            │  dummy-module
            │
            └───► index.js ─────┐
                                │
                  internals     │
                                │
                  ┌── index.js ◄┘
                  │
                  │   definition.js ◄┐
                  │                  │
                  ├─► leaf0.js ──────┤
A  ───────►  B    │                  │
   requires       └─► leaf1.js ──────┘

What's going on?

Looking at the benchmarks that are having trouble, we have 2 categories:

  1. Logs, which do tracer init then log some messages, the latter of which comparatively takes less time (so we're observing startup time more than log time, unless log costs explode for some reason)
  2. Startup with tracer

The startup issue is interesting, because we have 4 startup scenarios here, but only 1 fails.
Looking at the code (benchmarks/sirun/startup/startup-test.js):

/*
  startup-with-tracer has
    USE_TRACER=1
    EVERYTHING=0
*/
/*
  no issues with
    USE_TRACER=1
    EVERYTHING=1
*/

if (Number(process.env.USE_TRACER)) {
  require('../../..').init()
}

if (Number(process.env.EVERYTHING)) {
  const json = require('../../../package.json')
  for (const pkg in json.dependencies) {
    require(pkg)
  }
  for (const devPkg in json.devDependencies) {
    if (devPkg !== '@types/node') {
      require(devPkg)
    }
  }
}

My theory: we have issues in this benchmark, and not in the one where we load the tracer and everything because:

  1. there's some perf loss with the growing module count
  2. it's hidden when we load EVERYTHING, because EVERYTHING goes over the threshold we're observing anyway

@github-actions
Copy link

github-actions bot commented May 12, 2023

Overall package size

Self size: 4.12 MB
Deduped: 58.25 MB
No deduping: 58.3 MB

Dependency sizes

name version self size total size
@datadog/pprof 2.2.1 14.24 MB 15.12 MB
@datadog/native-iast-taint-tracking 1.4.1 14.85 MB 14.86 MB
@datadog/native-appsec 3.1.0 13.31 MB 13.32 MB
protobufjs 7.1.2 2.76 MB 6.55 MB
@datadog/native-iast-rewriter 2.0.1 2.09 MB 2.1 MB
@datadog/native-metrics 2.0.0 898.77 kB 1.3 MB
opentracing 0.14.7 194.81 kB 194.81 kB
semver 7.3.8 88.2 kB 118.6 kB
@datadog/sketches-js 2.1.0 109.9 kB 109.9 kB
lodash.sortby 4.7.0 75.76 kB 75.76 kB
lru-cache 7.14.0 74.95 kB 74.95 kB
ipaddr.js 2.0.1 59.52 kB 59.52 kB
ignore 5.2.0 48.87 kB 48.87 kB
import-in-the-middle 1.3.5 34.34 kB 38.81 kB
istanbul-lib-coverage 3.2.0 29.34 kB 29.34 kB
retry 0.10.1 27.44 kB 27.44 kB
lodash.uniq 4.5.0 25.01 kB 25.01 kB
limiter 1.1.5 23.17 kB 23.17 kB
lodash.kebabcase 4.1.1 17.75 kB 17.75 kB
lodash.pick 4.4.0 16.33 kB 16.33 kB
node-abort-controller 3.0.1 14.33 kB 14.33 kB
crypto-randomuuid 1.0.0 11.18 kB 11.18 kB
diagnostics_channel 1.1.0 7.07 kB 7.07 kB
path-to-regexp 0.1.7 6.78 kB 6.78 kB
koalas 1.0.2 6.47 kB 6.47 kB
methods 1.1.2 5.29 kB 5.29 kB
module-details-from-path 1.0.3 4.47 kB 4.47 kB

🤖 This report was automatically generated by heaviest-objects-in-the-universe

@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Merging #3146 (8990942) into master (cad3d92) will decrease coverage by 0.02%.
The diff coverage is 78.94%.

@@            Coverage Diff             @@
##           master    #3146      +/-   ##
==========================================
- Coverage   86.98%   86.97%   -0.02%     
==========================================
  Files         322      327       +5     
  Lines       11763    11782      +19     
  Branches       33       33              
==========================================
+ Hits        10232    10247      +15     
- Misses       1531     1535       +4     
Impacted Files Coverage Δ
packages/dd-trace/src/plugin_manager.js 98.41% <ø> (ø)
packages/dd-trace/src/dummy-module/index.js 66.66% <66.66%> (ø)
.../dd-trace/src/dummy-module/internals/definition.js 66.66% <66.66%> (ø)
packages/dd-trace/src/index.js 83.33% <66.66%> (-16.67%) ⬇️
...kages/dd-trace/src/dummy-module/internals/index.js 100.00% <100.00%> (ø)
...kages/dd-trace/src/dummy-module/internals/leaf0.js 100.00% <100.00%> (ø)
...kages/dd-trace/src/dummy-module/internals/leaf1.js 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jbertran jbertran force-pushed the jbertran/test-module-cache-weirdness branch from 994d094 to 16a1b6b Compare May 12, 2023 08:30
@pr-commenter
Copy link

pr-commenter bot commented May 12, 2023

Benchmarks

Comparing candidate commit 8990942 in PR branch jbertran/test-module-cache-weirdness with baseline commit cad3d92 in branch master.

Found 0 performance improvements and 10 performance regressions! Performance is the same for 436 metrics, 26 unstable metrics.

scenario:log-without-log-16

  • 🟥 execution_time [+8.680ms; +10.852ms] or [+5.026%; +6.283%]

scenario:log-with-debug-16

  • 🟥 cpu_user_time [+8.924ms; +16.561ms] or [+6.029%; +11.189%]
  • 🟥 execution_time [+9.336ms; +12.980ms] or [+5.364%; +7.458%]

scenario:log-skip-log-16

  • 🟥 execution_time [+9.466ms; +11.509ms] or [+5.446%; +6.622%]

scenario:startup-with-tracer-16

  • 🟥 execution_time [+9.225ms; +10.208ms] or [+5.736%; +6.347%]

scenario:log-without-log-18

  • 🟥 execution_time [+9.570ms; +10.943ms] or [+5.427%; +6.205%]

scenario:log-with-error-18

  • 🟥 execution_time [+9.532ms; +10.414ms] or [+5.367%; +5.863%]

scenario:log-with-debug-18

  • 🟥 execution_time [+9.975ms; +11.380ms] or [+5.625%; +6.418%]

scenario:log-skip-log-18

  • 🟥 execution_time [+10.300ms; +12.828ms] or [+5.791%; +7.213%]

scenario:startup-with-tracer-18

  • 🟥 execution_time [+9.833ms; +10.361ms] or [+5.930%; +6.248%]

@jbertran jbertran changed the title Ignore - add bare minimum structure introduced by service naming Minimum viable example - benchmark issues May 12, 2023
@jbertran jbertran force-pushed the jbertran/test-module-cache-weirdness branch from 68bb099 to 8990942 Compare May 15, 2023 08:59
@jbertran jbertran mentioned this pull request May 15, 2023
@jbertran
Copy link
Contributor Author

We're reasonably certain the benchmarks aren't representative. This is no longer needed.

@jbertran jbertran closed this May 16, 2023
@tlhunter tlhunter deleted the jbertran/test-module-cache-weirdness branch January 19, 2024 22:20
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.

1 participant