From 877dcdfc3a0e8307990d9222d140793937ae6443 Mon Sep 17 00:00:00 2001 From: Benedikt Meurer Date: Fri, 18 Feb 2022 16:27:57 +0100 Subject: [PATCH] [inspector] Don't hold on to Script objects strongly for caching. Previously we'd hold on to Script objects strongly after they are considered unreachable by V8 itself, and keep them around for the V8DebuggerAgent cache (whose upper limit can be controlled with a parameter to `Debugger.enable`). This CL changes that to instead copy out the script source and the WebAssembly bytecode (depending on whether it's JavaScript or Wasm) to the C++ heap and keep it cached there. Fixed: chromium:1295659 Bug: chromium:1246884 Change-Id: Idfcd7172715eafca6b011826ae03a573d58803f2 Doc: https://bit.ly/v8-inspector-script-caching Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3472082 Reviewed-by: Jaroslav Sevcik Reviewed-by: Michael Lippautz Commit-Queue: Benedikt Meurer Cr-Commit-Position: refs/heads/main@{#79217} --- src/inspector/v8-debugger-agent-impl.cc | 44 ++++++++++++++----- src/inspector/v8-debugger-agent-impl.h | 11 ++++- ...possible-breakpoints-after-gc-expected.txt | 6 +-- .../get-possible-breakpoints-after-gc.js | 3 -- .../set-breakpoint-after-gc-expected.txt | 4 +- 5 files changed, 46 insertions(+), 22 deletions(-) diff --git a/src/inspector/v8-debugger-agent-impl.cc b/src/inspector/v8-debugger-agent-impl.cc index 3dd35a07f6..a865e29c21 100644 --- a/src/inspector/v8-debugger-agent-impl.cc +++ b/src/inspector/v8-debugger-agent-impl.cc @@ -435,7 +435,7 @@ Response V8DebuggerAgentImpl::disable() { resetBlackboxedStateCache(); m_skipList.clear(); m_scripts.clear(); - m_cachedScriptIds.clear(); + m_cachedScripts.clear(); m_cachedScriptSize = 0; for (const auto& it : m_debuggerBreakpointIdToBreakpointId) { v8::debug::RemoveBreakpoint(m_isolate, it.first); @@ -1072,8 +1072,20 @@ Response V8DebuggerAgentImpl::getScriptSource( Maybe* bytecode) { if (!enabled()) return Response::ServerError(kDebuggerNotEnabled); ScriptsMap::iterator it = m_scripts.find(scriptId); - if (it == m_scripts.end()) + if (it == m_scripts.end()) { + auto cachedScriptIt = + std::find_if(m_cachedScripts.begin(), m_cachedScripts.end(), + [&scriptId](const CachedScript& cachedScript) { + return cachedScript.scriptId == scriptId; + }); + if (cachedScriptIt != m_cachedScripts.end()) { + *scriptSource = cachedScriptIt->source; + *bytecode = protocol::Binary::fromSpan(cachedScriptIt->bytecode.data(), + cachedScriptIt->bytecode.size()); + return Response::Success(); + } return Response::ServerError("No script for id: " + scriptId.utf8()); + } *scriptSource = it->second->source(0); #if V8_ENABLE_WEBASSEMBLY v8::MemorySpan span; @@ -1949,23 +1961,31 @@ void V8DebuggerAgentImpl::reset() { resetBlackboxedStateCache(); m_skipList.clear(); m_scripts.clear(); - m_cachedScriptIds.clear(); + m_cachedScripts.clear(); m_cachedScriptSize = 0; } void V8DebuggerAgentImpl::ScriptCollected(const V8DebuggerScript* script) { DCHECK_NE(m_scripts.find(script->scriptId()), m_scripts.end()); - m_cachedScriptIds.push_back(script->scriptId()); - // TODO(alph): Properly calculate size when sources are one-byte strings. - m_cachedScriptSize += script->length() * sizeof(uint16_t); + std::vector bytecode; +#if V8_ENABLE_WEBASSEMBLY + v8::MemorySpan span; + if (script->wasmBytecode().To(&span)) { + bytecode.reserve(span.size()); + bytecode.insert(bytecode.begin(), span.data(), span.data() + span.size()); + } +#endif + CachedScript cachedScript{script->scriptId(), script->source(0), + std::move(bytecode)}; + m_cachedScriptSize += cachedScript.size(); + m_cachedScripts.push_back(std::move(cachedScript)); + m_scripts.erase(script->scriptId()); while (m_cachedScriptSize > m_maxScriptCacheSize) { - const String16& scriptId = m_cachedScriptIds.front(); - size_t scriptSize = m_scripts[scriptId]->length() * sizeof(uint16_t); - DCHECK_GE(m_cachedScriptSize, scriptSize); - m_cachedScriptSize -= scriptSize; - m_scripts.erase(scriptId); - m_cachedScriptIds.pop_front(); + const CachedScript& cachedScript = m_cachedScripts.front(); + DCHECK_GE(m_cachedScriptSize, cachedScript.size()); + m_cachedScriptSize -= cachedScript.size(); + m_cachedScripts.pop_front(); } } diff --git a/src/inspector/v8-debugger-agent-impl.h b/src/inspector/v8-debugger-agent-impl.h index 6a154328dc..93f43e0386 100644 --- a/src/inspector/v8-debugger-agent-impl.h +++ b/src/inspector/v8-debugger-agent-impl.h @@ -226,7 +226,16 @@ class V8DebuggerAgentImpl : public protocol::Debugger::Backend { size_t m_maxScriptCacheSize = 0; size_t m_cachedScriptSize = 0; - std::deque m_cachedScriptIds; + struct CachedScript { + String16 scriptId; + String16 source; + std::vector bytecode; + + size_t size() const { + return source.length() * sizeof(UChar) + bytecode.size(); + } + }; + std::deque m_cachedScripts; using BreakReason = std::pair>; diff --git a/test/inspector/debugger/get-possible-breakpoints-after-gc-expected.txt b/test/inspector/debugger/get-possible-breakpoints-after-gc-expected.txt index 6968ed3eab..4362e8dd6a 100644 --- a/test/inspector/debugger/get-possible-breakpoints-after-gc-expected.txt +++ b/test/inspector/debugger/get-possible-breakpoints-after-gc-expected.txt @@ -1,7 +1,5 @@ Checks if we keep alive breakpoint information for top-level functions when calling getPossibleBreakpoints. -Result of get possible breakpoints in topLevel.js -[{"scriptId":"3","lineNumber":0,"columnNumber":0},{"scriptId":"3","lineNumber":0,"columnNumber":8,"type":"call"},{"scriptId":"3","lineNumber":0,"columnNumber":43,"type":"return"}] Result of get possible breakpoints in moduleFunc.js -[{"scriptId":"5","lineNumber":0,"columnNumber":22},{"scriptId":"5","lineNumber":0,"columnNumber":30,"type":"call"},{"scriptId":"5","lineNumber":0,"columnNumber":63,"type":"return"},{"scriptId":"5","lineNumber":0,"columnNumber":64,"type":"return"}] +[{"scriptId":"3","lineNumber":0,"columnNumber":22},{"scriptId":"3","lineNumber":0,"columnNumber":30,"type":"call"},{"scriptId":"3","lineNumber":0,"columnNumber":63,"type":"return"},{"scriptId":"3","lineNumber":0,"columnNumber":64,"type":"return"}] Result of get possible breakpoints in mixedFunctions.js -[{"scriptId":"7","lineNumber":0,"columnNumber":15,"type":"return"},{"scriptId":"7","lineNumber":1,"columnNumber":2},{"scriptId":"7","lineNumber":1,"columnNumber":10,"type":"call"},{"scriptId":"7","lineNumber":2,"columnNumber":0,"type":"return"}] +[{"scriptId":"5","lineNumber":0,"columnNumber":15,"type":"return"},{"scriptId":"5","lineNumber":1,"columnNumber":2},{"scriptId":"5","lineNumber":1,"columnNumber":10,"type":"call"},{"scriptId":"5","lineNumber":2,"columnNumber":0,"type":"return"}] diff --git a/test/inspector/debugger/get-possible-breakpoints-after-gc.js b/test/inspector/debugger/get-possible-breakpoints-after-gc.js index 097d0b99af..da281a47fc 100644 --- a/test/inspector/debugger/get-possible-breakpoints-after-gc.js +++ b/test/inspector/debugger/get-possible-breakpoints-after-gc.js @@ -17,7 +17,6 @@ const callGarbageCollector = ` %CollectGarbage(""); `; -const topLevelFunction = `console.log('This is a top level function')`; const moduleFunction = `function testFunc() { console.log('This is a module function') }`; let mixedFunctions = ` function A() {} @@ -33,8 +32,6 @@ function onDebuggerEnabled() { async function onExecutionContextCreated(messageObject) { executionContextId = messageObject.params.context.id; - await testGetPossibleBreakpoints( - executionContextId, topLevelFunction, 'topLevel.js'); await testGetPossibleBreakpoints( executionContextId, moduleFunction, 'moduleFunc.js'); await testGetPossibleBreakpoints( diff --git a/test/inspector/debugger/set-breakpoint-after-gc-expected.txt b/test/inspector/debugger/set-breakpoint-after-gc-expected.txt index 773f69990e..512ae69fcb 100644 --- a/test/inspector/debugger/set-breakpoint-after-gc-expected.txt +++ b/test/inspector/debugger/set-breakpoint-after-gc-expected.txt @@ -1,5 +1,5 @@ Checks if we keep alive breakpoint information for top-level functions. Result of setting breakpoint in topLevel.js -[{"scriptId":"3","lineNumber":0,"columnNumber":0}] +[] Result of setting breakpoint in moduleFunc.js -[{"scriptId":"5","lineNumber":0,"columnNumber":22}] \ No newline at end of file +[{"scriptId":"5","lineNumber":0,"columnNumber":22}]