Skip to content

Minimum viable example - benchmark issues#3146

Closed
jbertran wants to merge 3 commits into
masterfrom
jbertran/test-module-cache-weirdness
Closed

Minimum viable example - benchmark issues#3146
jbertran wants to merge 3 commits into
masterfrom
jbertran/test-module-cache-weirdness

Conversation

@jbertran

@jbertran jbertran commented May 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

github-actions Bot commented May 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

codecov Bot commented May 12, 2023 •
edited
Loading

Copy link
Copy Markdown

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

pr-commenter Bot commented May 12, 2023 •
edited
Loading

Copy link
Copy Markdown

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
Closed
@jbertran

jbertran commented May 16, 2023

Copy link
Copy Markdown
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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@jbertran