Andres Olivares (Gerrit)

unread,

Jan 2, 2025, 9:52:23 AMJan 2

to Benedikt Meurer, Eric Leese, V8 LUCI CQ, devtools-...@chromium.org, v8-re...@googlegroups.com

Attention needed from Benedikt Meurer and Eric Leese

Andres Olivares voted and added 1 comment

Votes added by Andres Olivares

Commit-Queue

+1

1 comment

Patchset-level comments

File-level comment, Patchset 7 (Latest):

Andres Olivares . resolved

PTAL

Open in Gerrit

Related details

Attention is currently required from:

Benedikt Meurer
Eric Leese

Submit Requirements:

Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Eric Leese (Gerrit)

unread,

Jan 2, 2025, 11:43:26 AMJan 2

to Andres Olivares, Benedikt Meurer, V8 LUCI CQ, devtools-...@chromium.org, v8-re...@googlegroups.com

Attention needed from Andres Olivares and Benedikt Meurer

Eric Leese voted and added 2 comments

Votes added by Eric Leese

Code-Review

+1

2 comments

File src/inspector/v8-console.cc

Line 458, Patchset 7 (Latest): v8::Isolate* isolate = info.GetIsolate();

Eric Leese . unresolved

Why do we get the isolate in two different ways? Can this be used by the call to consoleTimeStamp? Should it be m_inspector->isolate() instead? If they are different, can this line be moved inside the #ifdef?

Line 461, Patchset 7 (Latest): String16 args[6];

Eric Leese . unresolved

It appears this is only used in the #ifdef; can it be moved inside it?

Open in Gerrit

Related details

Attention is currently required from:

Andres Olivares
Benedikt Meurer

Submit Requirements:

Code-Review

No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Andres Olivares (Gerrit)

unread,

Jan 2, 2025, 11:59:10 AMJan 2

to Eric Leese, Benedikt Meurer, V8 LUCI CQ, devtools-...@chromium.org, v8-re...@googlegroups.com

Attention needed from Benedikt Meurer and Eric Leese

Andres Olivares added 2 comments

File src/inspector/v8-console.cc

Line 458, Patchset 7: v8::Isolate* isolate = info.GetIsolate();

Eric Leese . resolved

Why do we get the isolate in two different ways? Can this be used by the call to consoleTimeStamp? Should it be m_inspector->isolate() instead? If they are different, can this line be moved inside the #ifdef?

Andres Olivares

I think we can use either way. Done. Thanks!

Line 461, Patchset 7: String16 args[6];

Eric Leese . resolved

It appears this is only used in the #ifdef; can it be moved inside it?

Andres Olivares

Good call. Done.

Open in Gerrit

Related details

Attention is currently required from:

Benedikt Meurer
Eric Leese

Submit Requirements:

Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Eric Leese (Gerrit)

unread,

Jan 2, 2025, 12:32:28 PMJan 2

to Andres Olivares, Benedikt Meurer, V8 LUCI CQ, devtools-...@chromium.org, v8-re...@googlegroups.com

Attention needed from Andres Olivares and Benedikt Meurer

Eric Leese voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Andres Olivares
Benedikt Meurer

Submit Requirements:

Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Benedikt Meurer (Gerrit)

unread,

Jan 3, 2025, 1:50:12 AMJan 3

to Andres Olivares, Eric Leese, V8 LUCI CQ, devtools-...@chromium.org, v8-re...@googlegroups.com

Attention needed from Andres Olivares

Benedikt Meurer added 1 comment

File src/inspector/v8-console.cc

Line 456, Patchset 8 (Latest):void V8Console::TimeStamp(const v8::debug::ConsoleCallArguments& info,

Benedikt Meurer . unresolved

This is pretty costly, since you do a lot of string copying independent of whether tracing is turned on, and in case tracing is on, you still do quite a few string copies. How about something like this instead?

```cpp
void V8Console::TimeStamp(const v8::debug::ConsoleCallArguments& info,
                          const v8::debug::ConsoleContext& consoleContext) {
  TRACE_EVENT_BEGIN0(TRACE_DISABLED_BY_DEFAULT("v8.inspector"),
                     "V8Console::TimeStamp");
  v8::Isolate* isolate = m_inspector->isolate();
  ConsoleHelper helper(info, consoleContext, m_inspector);
  v8::Local<v8::String> label = helper.firstArgToString();
  m_inspector->client()->consoleTimeStamp(isolate, label);
  TRACE_EVENT_END2(
      TRACE_DISABLED_BY_DEFAULT("v8.inspector"), "V8Console::TimeStamp", "name",
      TRACE_STR_COPY(toProtocolString(isolate, label).utf8().c_str()), "args",
      ([&]() -> std::unique_ptr<v8::tracing::TracedValue> {
        static const char* kNames[] = {"name",      "start",      "end",
                                       "trackName", "trackGroup", "color"};
        auto args = v8::tracing::TracedValue::Create();
        for (int i = 1; i < info.Length(); ++i) {
          auto name = kNames[i];
          auto value = info[i];
          if (value->IsNumber()) {
            args->SetDouble(name, value.As<v8::Number>()->Value());
          } else {
            args->SetString(name, toProtocolString(isolate, value).utf8());
          }
        }
        return args;
      }));
}
```

Open in Gerrit

Related details

Attention is currently required from:

Andres Olivares

Submit Requirements:

Code-Review

No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Andres Olivares (Gerrit)

unread,

Jan 3, 2025, 7:56:00 AMJan 3

to Eric Leese, Benedikt Meurer, V8 LUCI CQ, devtools-...@chromium.org, v8-re...@googlegroups.com

Attention needed from Benedikt Meurer and Eric Leese

Andres Olivares added 1 comment

File src/inspector/v8-console.cc

Line 456, Patchset 8:void V8Console::TimeStamp(const v8::debug::ConsoleCallArguments& info,

Benedikt Meurer . resolved

Andres Olivares

thanks! did something similar while trying to keep it simple with a single call to trace

Open in Gerrit

Related details

Attention is currently required from:

Benedikt Meurer
Eric Leese

Submit Requirements:

Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Benedikt Meurer (Gerrit)

unread,

Jan 3, 2025, 8:33:06 AMJan 3

to Andres Olivares, Eric Leese, V8 LUCI CQ, devtools-...@chromium.org, v8-re...@googlegroups.com

Attention needed from Andres Olivares and Eric Leese

Benedikt Meurer voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Andres Olivares
Eric Leese

Submit Requirements:

Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Andres Olivares (Gerrit)

unread,

Jan 7, 2025, 4:31:06 AMJan 7

to Benedikt Meurer, Eric Leese, V8 LUCI CQ, devtools-...@chromium.org, v8-re...@googlegroups.com

Attention needed from Benedikt Meurer and Eric Leese

Andres Olivares added 1 comment

Patchset-level comments

File-level comment, Patchset 10 (Latest):

Andres Olivares . resolved

I've lost your votes after refactoring to address the comments, PTAnotherL :)

Open in Gerrit

Related details

Attention is currently required from:

Benedikt Meurer
Eric Leese

Submit Requirements:

Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Benedikt Meurer (Gerrit)

unread,

Jan 7, 2025, 7:46:04 AMJan 7

to Andres Olivares, Eric Leese, V8 LUCI CQ, devtools-...@chromium.org, v8-re...@googlegroups.com

Attention needed from Andres Olivares and Eric Leese

Benedikt Meurer voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Andres Olivares
Eric Leese

Submit Requirements:

Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Benedikt Meurer (Gerrit)

unread,

Jan 7, 2025, 7:46:18 AMJan 7

to Andres Olivares, Eric Leese, V8 LUCI CQ, devtools-...@chromium.org, v8-re...@googlegroups.com

Attention needed from Andres Olivares and Eric Leese

Benedikt Meurer voted Commit-Queue+1

Commit-Queue

+1

Open in Gerrit

Related details

Attention is currently required from:

Andres Olivares
Eric Leese

Submit Requirements:

Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Eric Leese (Gerrit)

unread,

Jan 7, 2025, 12:28:40 PMJan 7

to Andres Olivares, V8 LUCI CQ, devtools-...@chromium.org, v8-re...@googlegroups.com

Attention needed from Andres Olivares

Eric Leese voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Andres Olivares

Submit Requirements:

Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Andres Olivares (Gerrit)

unread,

Jan 7, 2025, 12:30:19 PMJan 7

to Eric Leese, V8 LUCI CQ, devtools-...@chromium.org, v8-re...@googlegroups.com

Andres Olivares voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

V8 LUCI CQ (Gerrit)

unread,

Jan 7, 2025, 12:34:53 PMJan 7

to Andres Olivares, Eric Leese, Benedikt Meurer, devtools-...@chromium.org, v8-re...@googlegroups.com

V8 LUCI CQ submitted the change

Change information

Commit message:

Include extra parameters passed to console.timeStamp in trace event

Per https://docs.google.com/document/d/1AsQTakcWz5zH2UC7DdSmoyd3TQ0Q2JUsabHK_y_p1r8/

No user visible behavior is affected by this CL. The usage of the newly
traced data will be handled competely in the devtools frontend.

Bug: 377884142

Change-Id: I4570e983fe9399ffe96e26efa3304413564fb3cf

Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6110720

Commit-Queue: Andres Olivares <and...@chromium.org>

Reviewed-by: Eric Leese <le...@chromium.org>

Reviewed-by: Benedikt Meurer <bme...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#97972}

Files:

M src/inspector/v8-console.cc

Change size: S

Delta: 1 file changed, 25 insertions(+), 2 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Benedikt Meurer, +1 by Eric Leese

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

Include extra parameters passed to console.timeStamp in trace event [v8/v8 : main]

Andres Olivares (Gerrit)

Andres Olivares voted and added 1 comment

Votes added by Andres Olivares

1 comment

Related details

Eric Leese (Gerrit)

Eric Leese voted and added 2 comments

Votes added by Eric Leese

2 comments

Related details

Andres Olivares (Gerrit)

Andres Olivares added 2 comments

Related details

Eric Leese (Gerrit)

Eric Leese voted Code-Review+1

Related details

Benedikt Meurer (Gerrit)

Benedikt Meurer added 1 comment

Related details

Andres Olivares (Gerrit)

Andres Olivares added 1 comment

Related details

Benedikt Meurer (Gerrit)

Benedikt Meurer voted Code-Review+1

Related details

Andres Olivares (Gerrit)

Andres Olivares added 1 comment

Related details

Benedikt Meurer (Gerrit)

Benedikt Meurer voted Code-Review+1

Related details

Benedikt Meurer (Gerrit)

Benedikt Meurer voted Commit-Queue+1

Related details

Eric Leese (Gerrit)

Eric Leese voted Code-Review+1

Related details

Andres Olivares (Gerrit)

Andres Olivares voted Commit-Queue+2

Related details

V8 LUCI CQ (Gerrit)

V8 LUCI CQ submitted the change

Change information