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.

Sign up for GitHub

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

src: enable detailed source positions in V8 #24274

Closed
wants to merge 2 commits into from
Closed

src: enable detailed source positions in V8 #24274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dc102e8
deps: backport 073073b4f1 from upstream V8
hashseed Nov 9, 2018
aa32194
src: enable detailed source positions in V8
hashseed Nov 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion common.gypi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@

# Reset this number to 0 on major V8 upgrades.
# Increment by one for each non-official patch applied to deps/v8.
'v8_embedder_string': '-node.10',
'v8_embedder_string': '-node.11',

# Enable disassembler for `--print-code` v8 options
'v8_enable_disassembler': 1,
Expand Down
6 changes: 6 additions & 0 deletions deps/v8/include/v8-profiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,12 @@ class V8_EXPORT CpuProfiler {
V8_DEPRECATED("Use Isolate::SetIdle(bool) instead.",
void SetIdle(bool is_idle));

/**
* Generate more detailed source positions to code objects. This results in
* better results when mapping profiling samples to script source.
*/
static void UseDetailedSourcePositionsForProfiling(Isolate* isolate);

private:
CpuProfiler();
~CpuProfiler();
Expand Down
5 changes: 5 additions & 0 deletions deps/v8/src/api.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10132,6 +10132,11 @@ void CpuProfiler::SetIdle(bool is_idle) {
isolate->SetIdle(is_idle);
}

void CpuProfiler::UseDetailedSourcePositionsForProfiling(Isolate* isolate) {
reinterpret_cast<i::Isolate*>(isolate)
->set_detailed_source_positions_for_profiling(true);
}

uintptr_t CodeEvent::GetCodeStartAddress() {
return reinterpret_cast<i::CodeEvent*>(this)->code_start_address;
}
Expand Down
3 changes: 2 additions & 1 deletion deps/v8/src/isolate.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3257,7 +3257,8 @@ bool Isolate::use_optimizer() {
}

bool Isolate::NeedsDetailedOptimizedCodeLineInfo() const {
return NeedsSourcePositionsForProfiling() || FLAG_detailed_line_info;
return NeedsSourcePositionsForProfiling() ||
detailed_source_positions_for_profiling();
}

bool Isolate::NeedsSourcePositionsForProfiling() const {
Expand Down
3 changes: 2 additions & 1 deletion deps/v8/src/isolate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,7 +553,8 @@ typedef std::vector<HeapObject*> DebugObjectCache;
V(int, last_console_context_id, 0) \
V(v8_inspector::V8Inspector*, inspector, nullptr) \
V(bool, next_v8_call_is_safe_for_termination, false) \
V(bool, only_terminate_in_safe_scope, false)
V(bool, only_terminate_in_safe_scope, false) \
V(bool, detailed_source_positions_for_profiling, FLAG_detailed_line_info)

#define THREAD_LOCAL_TOP_ACCESSOR(type, name) \
inline void set_##name(type v) { thread_local_top_.name##_ = v; } \
Expand Down
41 changes: 41 additions & 0 deletions deps/v8/test/cctest/test-cpu-profiler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
#include "src/objects-inl.h"
#include "src/profiler/cpu-profiler-inl.h"
#include "src/profiler/profiler-listener.h"
#include "src/source-position-table.h"
#include "src/utils.h"
#include "test/cctest/cctest.h"
#include "test/cctest/profiler-extension.h"
Expand Down Expand Up @@ -2544,6 +2545,46 @@ TEST(MultipleProfilers) {
profiler2->StopProfiling("2");
}

UNINITIALIZED_TEST(DetailedSourcePositionAPI) {
i::FLAG_detailed_line_info = false;
i::FLAG_allow_natives_syntax = true;
v8::Isolate::CreateParams create_params;
create_params.array_buffer_allocator = CcTest::array_buffer_allocator();
v8::Isolate* isolate = v8::Isolate::New(create_params);

const char* source =
"function fib(i) {"
" if (i <= 1) return 1; "
" return fib(i - 1) +"
" fib(i - 2);"
"}"
"fib(5);"
"%OptimizeFunctionOnNextCall(fib);"
"fib(5);"
"fib";
{
v8::Isolate::Scope isolate_scope(isolate);
v8::HandleScope handle_scope(isolate);
v8::Local<v8::Context> context = v8::Context::New(isolate);
v8::Context::Scope context_scope(context);
i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);

CHECK(!i_isolate->NeedsDetailedOptimizedCodeLineInfo());

int non_detailed_positions = GetSourcePositionEntryCount(i_isolate, source);

v8::CpuProfiler::UseDetailedSourcePositionsForProfiling(isolate);
CHECK(i_isolate->NeedsDetailedOptimizedCodeLineInfo());

int detailed_positions = GetSourcePositionEntryCount(i_isolate, source);

CHECK((non_detailed_positions == -1 && detailed_positions == -1) ||
non_detailed_positions < detailed_positions);
}

isolate->Dispose();
}

} // namespace test_cpu_profiler
} // namespace internal
} // namespace v8
1 change: 1 addition & 0 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2548,6 +2548,7 @@ Isolate* NewIsolate(ArrayBufferAllocator* allocator, uv_loop_t* event_loop) {
isolate->SetMicrotasksPolicy(MicrotasksPolicy::kExplicit);
isolate->SetFatalErrorHandler(OnFatalError);
isolate->SetAllowWasmCodeGenerationCallback(AllowWasmCodeGenerationCallback);
v8::CpuProfiler::UseDetailedSourcePositionsForProfiling(isolate);
Copy link
Contributor

@refack refack Nov 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Question: does this have a penalty? And even if not could this be conditional on inspector being attached or profiler run requested (I mean is it possible)?

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with only enabling this once the profiler run is requested is that functions that get optimized before profiling starts don't have the detailed line information. In the browser you can work around this by reloading the page with profiling enabled (devtools has a button for this) but for a running Node instance this is not really feasible, particularly if you want to debug some rarely-occurring thing. Unfortunately we see this in the wild quite a bit so having it on by default makes sense IMO.

The overhead is very low - on average it makes no difference, it the worst case a few percent increase in optimized compile time, but this is currently being moved off-thread anyway (plus the general performance boost of upgrading the V8 version is going to easily outweigh this).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation.

Copy link
Contributor

@refack refack Nov 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

In the browser you can work around this by reloading the page with profiling enabled

I'm thinking about this some more...
For node we have one main way to enable inspetor, via CLI args. There are two other ways which I don't have usage information about SIGUSR and inspector.open().

It might be a better balance to turn this on only for processes started with inspector listening, and save the performance cost for regular runs. IIUC the down side is less accurate code position information?

I'm ambivalent...

/CC @nodejs/performance

Copy link
Member

Choose a reason for hiding this comment

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

CpuProfiler can be also used without inspector, see e.g. NPM modules like v8-profiler. There are tools out in the wild which start profiling automatically for some time if they notice performance issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's also possible to use the CPU profiler directly through the V8 API without the inspector and there are some native modules that do this - that approach wouldn't work for them as far as I understand

Copy link
Member Author

Choose a reason for hiding this comment

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

The thoughts leading to this were:

  • It's hard to foresee whether the user is going to profile, both for Chrome and Node.
  • For Chrome, you can at least reload, with DevTools open.
  • Node.js does not have the reload option.
  • Adding more accurate data has a small one-time cost for the script. Chrome suffers a bit more for this as you pay more for optimizing compiles, possibly while starting up a page, and this occurs on every page navigation. Node.js does not encounter new scripts to compile and optimize as often.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that it's a tough call:
slightly slower compile time but for everyone ⚖️ improvement not available but only for profiling started out-of-band


return isolate;
}
Expand Down