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

--perf-basic-prof-only-functions flag is broken since v8 8.6 #50225

Closed
lukealbao opened this issue Oct 17, 2023 · 4 comments · Fixed by #50302
Closed

--perf-basic-prof-only-functions flag is broken since v8 8.6 #50225

lukealbao opened this issue Oct 17, 2023 · 4 comments · Fixed by #50302
Labels
v8 engine Issues and PRs related to the V8 dependency.

Comments

@lukealbao
Copy link
Contributor

Version

16.20.0

Platform

Linux [hostname] 5.4.254-170.358.amzn2.x86_64 #1 SMP Wed Sep 6 21:10:58 UTC 2023 x86_64 GNU/Linux

Subsystem

v8

What steps will reproduce the bug?

The --perf-basic-prof-only-functions flag is documented to be safe for production, and allows perf(1) to generate reports with JIT symbols. The emitted symbol table includes only addresses for js functions. This no longer happens, but it does for the more expensive --perf-basic-prof flag.

This script can reproduce the error:

#! /usr/bin/env bash

# Create a repro js script.
cat <<'eof' > /tmp/repro.js
  process.stdout.write(`${process.pid}`);

  const reproRe = /reproRe/gi;

  function reproFnOptimized() {
    for (let i = 0; i < 10000; i++) {
      const match = reproRe.exec(Math.random().toString());
    }
  }
  function reproFnInterpreted() {
      reproFnOptimized();
  }
  reproFnInterpreted();
  console.error(`[Using ${process.execArgv[0]}]: /tmp/perf-${process.pid}.map`);
eof

# Collect JIT symbols from the repro script.
basic_prof_pid="$(node --perf-basic-prof /tmp/repro.js)"
functions_only_pid="$(node --perf-basic-prof-only-functions /tmp/repro.js)"

# --perf-basic-prof emits function code kinds.
echo "=== Testing --perf-basic-prof output ==="
if ! grep -E --color 'reproFn\w+'  "/tmp/perf-${basic_prof_pid}.map"; then
  echo "--- (Repro Error): expected to find JS function symbols"
else
  echo '--- (Repro OK)'
fi

# --perf-basic-prof-only-functions does not emit JS functions, but does emit regex kinds.
echo "=== Testing --perf-basic-prof-only-functions output ==="
if grep -E --color 'reproFn\w+'  "/tmp/perf-${functions_only_pid}.map"; then
  echo "Error: expected not to find JS function symbols"
elif ! grep -E --color 'reproRe'  "/tmp/perf-${functions_only_pid}.map"; then
  echo "--- (Repro Error): expected to find RegEx symbols"
else
  echo '--- (Repro OK)'
fi

How often does it reproduce? Is there a required condition?

This bug is present in all versions that have v8 > 8.6. It's only applicable to Linux.

What is the expected behavior? Why is that the expected behavior?

Interpreted and optimized function frames should be logged.

What do you see instead?

No function frames, but there are regex frames.

Additional information

I originally opened this in nodejs/diagnostics#622.

@lukealbao
Copy link
Contributor Author

Upstream issue created in https://bugs.chromium.org/p/v8/issues/detail?id=14387, and tip-of-top fix submitted in https://chromium-review.googlesource.com/c/v8/v8/+/4950839.

@H4ad H4ad added the v8 engine Issues and PRs related to the V8 dependency. label Oct 18, 2023
@targos
Copy link
Member

targos commented Oct 18, 2023

Let's wait for resolution on the V8 side.

@brancz
Copy link

brancz commented Nov 18, 2023

I saw this patch was released in https://github.com/nodejs/node/releases/tag/v21.1.0

Is there an indication of when this patch will be available for v20 and v18 (perhaps even v16)?

@tmm1
Copy link

tmm1 commented Nov 16, 2024

It would be nice to backport to v20.

There was an attempt here: #50345

Which got rolled into #50077, but then that PR was abandoned when there were some build failures related to the backport.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants