Commit 317b72b2 authored by Peter Marshall's avatar Peter Marshall Committed by Commit Bot

[tracing] Separate tracing implementations and add perfetto tests

Previously both tracing implementations would be run side-by-side when
perfetto was enabled with the V8_USE_PERFETTO build flag. This CL
makes them run separately.

Both implementations now use the trace file provided by the user in D8
or the default v8_trace.json.

Add tests for perfetto events (which must be tested differently
due to the proto output format).

Drive-by fix: Fix pass-by non-const ref in GetJSONStrings.

Remove the TraceEvent struct for testing; we can just store a copy of
the protobuf directly.

Bug: v8:8339
Change-Id: Id50003e0f96e44b99a63a26693da6bdaca989504
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1702619Reviewed-by: 's avatarYang Guo <yangguo@chromium.org>
Reviewed-by: 's avatarJakob Gruber <jgruber@chromium.org>
Commit-Queue: Peter Marshall <petermarshall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#62810}
parent bc33f5ae
......@@ -3323,28 +3323,25 @@ int Shell::Main(int argc, char* argv[]) {
std::unique_ptr<platform::tracing::TracingController> tracing;
std::ofstream trace_file;
#ifdef V8_USE_PERFETTO
std::ofstream perfetto_trace_file;
#endif // V8_USE_PERFETTO
if (options.trace_enabled && !i::FLAG_verify_predictable) {
tracing = base::make_unique<platform::tracing::TracingController>();
trace_file.open(options.trace_path ? options.trace_path : "v8_trace.json");
DCHECK(trace_file.good());
platform::tracing::TraceBuffer* trace_buffer =
platform::tracing::TraceBuffer::CreateTraceBufferRingBuffer(
platform::tracing::TraceBuffer::kRingBufferChunks,
platform::tracing::TraceWriter::CreateJSONTraceWriter(trace_file));
tracing->Initialize(trace_buffer);
#ifdef V8_USE_PERFETTO
// Set up the in-process backend that the tracing controller will connect
// to.
perfetto::TracingInitArgs init_args;
init_args.backends = perfetto::BackendType::kInProcessBackend;
perfetto::Tracing::Initialize(init_args);
perfetto_trace_file.open("v8_perfetto_trace.json");
DCHECK(trace_file.good());
tracing->InitializeForPerfetto(&perfetto_trace_file);
tracing->InitializeForPerfetto(&trace_file);
#else
platform::tracing::TraceBuffer* trace_buffer =
platform::tracing::TraceBuffer::CreateTraceBufferRingBuffer(
platform::tracing::TraceBuffer::kRingBufferChunks,
platform::tracing::TraceWriter::CreateJSONTraceWriter(trace_file));
tracing->Initialize(trace_buffer);
#endif // V8_USE_PERFETTO
}
......
......@@ -92,6 +92,7 @@ void TracingController::InitializeForPerfetto(std::ostream* output_stream) {
output_stream_ = output_stream;
DCHECK_NOT_NULL(output_stream);
DCHECK(output_stream->good());
mutex_.reset(new base::Mutex());
}
void TracingController::SetTraceEventListenerForTesting(
......@@ -146,7 +147,9 @@ void AddArgsToTraceProto(
case TRACE_VALUE_TYPE_POINTER:
arg->set_pointer_value(arg_value.as_uint);
break;
// TODO(petermarshall): Treat copy strings specially.
// There is no difference between copy strings and regular strings for
// Perfetto; the set_string_value(const char*) API will copy the string
// into the protobuf by default.
case TRACE_VALUE_TYPE_COPY_STRING:
case TRACE_VALUE_TYPE_STRING:
arg->set_string_value(arg_value.as_string);
......@@ -216,7 +219,8 @@ uint64_t TracingController::AddTraceEventWithTimestamp(
AddArgsToTraceProto(trace_event, num_args, arg_names, arg_types,
arg_values, arg_convertables);
});
#endif // V8_USE_PERFETTO
return 0;
#else
uint64_t handle = 0;
if (recording_.load(std::memory_order_acquire)) {
......@@ -232,6 +236,7 @@ uint64_t TracingController::AddTraceEventWithTimestamp(
}
}
return handle;
#endif // V8_USE_PERFETTO
}
void TracingController::UpdateTraceEventDuration(
......@@ -251,11 +256,12 @@ void TracingController::UpdateTraceEventDuration(
trace_event->set_process_id(base::OS::GetCurrentProcessId());
trace_event->set_thread_timestamp(cpu_now_us);
});
#endif // V8_USE_PERFETTO
#else
TraceObject* trace_object = trace_buffer_->GetEventByHandle(handle);
if (!trace_object) return;
trace_object->UpdateDuration(now_us, cpu_now_us);
#endif // V8_USE_PERFETTO
}
const char* TracingController::GetCategoryGroupName(
......@@ -317,7 +323,6 @@ void TracingController::StopTracing() {
if (!recording_.compare_exchange_strong(expected, false)) {
return;
}
DCHECK(trace_buffer_);
UpdateCategoryGroupEnabledFlags();
std::unordered_set<v8::TracingController::TraceStateObserver*> observers_copy;
{
......@@ -339,13 +344,14 @@ void TracingController::StopTracing() {
if (listener_for_testing_) listener_for_testing_->ParseFromArray(trace);
json_listener_.reset();
#endif // V8_USE_PERFETTO
#else
{
base::MutexGuard lock(mutex_.get());
DCHECK(trace_buffer_);
trace_buffer_->Flush();
}
#endif // V8_USE_PERFETTO
}
void TracingController::UpdateCategoryGroupEnabledFlag(size_t category_index) {
......
......@@ -32,7 +32,9 @@ enum CategoryGroupEnabledFlags {
kEnabledForETWExport_CategoryGroupEnabledFlags = 1 << 3,
};
// By default, const char* asrgument values are assumed to have long-lived scope
// TODO(petermarshall): Remove with the old tracing implementation - Perfetto
// copies const char* arguments by default.
// By default, const char* argument values are assumed to have long-lived scope
// and will not be copied. Use this macro to force a const char* to be copied.
#define TRACE_STR_COPY(str) v8::internal::tracing::TraceStringWithCopy(str)
......
......@@ -306,6 +306,7 @@ int main(int argc, char* argv[]) {
}
#ifdef V8_USE_PERFETTO
// Set up the in-process backend that the tracing controller will connect to.
perfetto::TracingInitArgs init_args;
init_args.backends = perfetto::BackendType::kInProcessBackend;
perfetto::Tracing::Initialize(init_args);
......
......@@ -225,6 +225,21 @@ TEST(TestJSONTraceWriterWithCustomtag) {
CHECK_EQ(expected_trace_str, trace_str);
}
void GetJSONStrings(std::vector<std::string>* ret, const std::string& str,
const std::string& param, const std::string& start_delim,
const std::string& end_delim) {
size_t pos = str.find(param);
while (pos != std::string::npos) {
size_t start_pos = str.find(start_delim, pos + param.length());
size_t end_pos = str.find(end_delim, start_pos + 1);
CHECK_NE(start_pos, std::string::npos);
CHECK_NE(end_pos, std::string::npos);
ret->push_back(str.substr(start_pos + 1, end_pos - start_pos - 1));
pos = str.find(param, pos + 1);
}
}
#ifndef V8_USE_PERFETTO
TEST(TestTracingController) {
v8::Platform* old_platform = i::V8::GetCurrentPlatform();
std::unique_ptr<v8::Platform> default_platform(
......@@ -261,21 +276,6 @@ TEST(TestTracingController) {
i::V8::SetPlatformForTesting(old_platform);
}
void GetJSONStrings(
std::vector<std::string>& ret, // NOLINT(runtime/references)
std::string str, std::string param, std::string start_delim,
std::string end_delim) {
size_t pos = str.find(param);
while (pos != std::string::npos) {
size_t start_pos = str.find(start_delim, pos + param.length());
size_t end_pos = str.find(end_delim, start_pos + 1);
CHECK_NE(start_pos, std::string::npos);
CHECK_NE(end_pos, std::string::npos);
ret.push_back(str.substr(start_pos + 1, end_pos - start_pos - 1));
pos = str.find(param, pos + 1);
}
}
TEST(TestTracingControllerMultipleArgsAndCopy) {
std::ostringstream stream, perfetto_stream;
uint64_t aa = 11;
......@@ -371,9 +371,9 @@ TEST(TestTracingControllerMultipleArgsAndCopy) {
std::string trace_str = stream.str();
std::vector<std::string> all_args, all_names, all_cats;
GetJSONStrings(all_args, trace_str, "\"args\"", "{", "}");
GetJSONStrings(all_names, trace_str, "\"name\"", "\"", "\"");
GetJSONStrings(all_cats, trace_str, "\"cat\"", "\"", "\"");
GetJSONStrings(&all_args, trace_str, "\"args\"", "{", "}");
GetJSONStrings(&all_names, trace_str, "\"name\"", "\"", "\"");
GetJSONStrings(&all_cats, trace_str, "\"cat\"", "\"", "\"");
CHECK_EQ(all_args.size(), 24u);
CHECK_EQ(all_args[0], "\"aa\":11");
......@@ -407,6 +407,7 @@ TEST(TestTracingControllerMultipleArgsAndCopy) {
CHECK_EQ(all_args[22], "\"a1\":[42,42]");
CHECK_EQ(all_args[23], "\"a1\":[42,42],\"a2\":[123,123]");
}
#endif // !V8_USE_PERFETTO
namespace {
......@@ -553,35 +554,14 @@ TEST(AddTraceEventMultiThreaded) {
#ifdef V8_USE_PERFETTO
struct TraceEvent {
std::string name;
int64_t timestamp;
int32_t phase;
int32_t thread_id;
int64_t duration;
int64_t thread_duration;
std::string scope;
uint64_t id;
uint32_t flags;
std::string category_group_name;
int32_t process_id;
int64_t thread_timestamp;
uint64_t bind_id;
};
using TraceEvent = ::perfetto::protos::ChromeTraceEvent;
class TestListener : public TraceEventListener {
public:
void ProcessPacket(const ::perfetto::protos::TracePacket& packet) {
for (const ::perfetto::protos::ChromeTraceEvent& event :
packet.chrome_events().trace_events()) {
TraceEvent trace_event{event.name(), event.timestamp(),
event.phase(), event.thread_id(),
event.duration(), event.thread_duration(),
event.scope(), event.id(),
event.flags(), event.category_group_name(),
event.process_id(), event.thread_timestamp(),
event.bind_id()};
events_.push_back(trace_event);
events_.push_back(event);
}
}
......@@ -606,10 +586,6 @@ class TracingTestHarness {
static_cast<v8::platform::DefaultPlatform*>(default_platform_.get())
->SetTracingController(std::move(tracing));
MockTraceWriter* writer = new MockTraceWriter();
TraceBuffer* ring_buffer =
TraceBuffer::CreateTraceBufferRingBuffer(1, writer);
tracing_controller_->Initialize(ring_buffer);
tracing_controller_->InitializeForPerfetto(&perfetto_json_stream_);
tracing_controller_->SetTraceEventListenerForTesting(&listener_);
}
......@@ -655,52 +631,284 @@ TEST(Perfetto) {
harness.StopTracing();
TraceEvent* event = harness.get_event(0);
int32_t thread_id = event->thread_id;
int32_t process_id = event->process_id;
CHECK_EQ("test1", event->name);
CHECK_EQ(TRACE_EVENT_PHASE_BEGIN, event->phase);
int64_t timestamp = event->timestamp;
int32_t thread_id = event->thread_id();
int32_t process_id = event->process_id();
CHECK_EQ("test1", event->name());
CHECK_EQ(TRACE_EVENT_PHASE_BEGIN, event->phase());
int64_t timestamp = event->timestamp();
event = harness.get_event(1);
CHECK_EQ("test2", event->name);
CHECK_EQ(TRACE_EVENT_PHASE_BEGIN, event->phase);
CHECK_EQ(thread_id, event->thread_id);
CHECK_EQ(process_id, event->process_id);
CHECK_GE(event->timestamp, timestamp);
timestamp = event->timestamp;
CHECK_EQ("test2", event->name());
CHECK_EQ(TRACE_EVENT_PHASE_BEGIN, event->phase());
CHECK_EQ(thread_id, event->thread_id());
CHECK_EQ(process_id, event->process_id());
CHECK_GE(event->timestamp(), timestamp);
timestamp = event->timestamp();
event = harness.get_event(2);
CHECK_EQ("test3", event->name);
CHECK_EQ(TRACE_EVENT_PHASE_BEGIN, event->phase);
CHECK_EQ(thread_id, event->thread_id);
CHECK_EQ(process_id, event->process_id);
CHECK_GE(event->timestamp, timestamp);
timestamp = event->timestamp;
CHECK_EQ("test3", event->name());
CHECK_EQ(TRACE_EVENT_PHASE_BEGIN, event->phase());
CHECK_EQ(thread_id, event->thread_id());
CHECK_EQ(process_id, event->process_id());
CHECK_GE(event->timestamp(), timestamp);
timestamp = event->timestamp();
event = harness.get_event(3);
CHECK_EQ(TRACE_EVENT_PHASE_END, event->phase);
CHECK_EQ(thread_id, event->thread_id);
CHECK_EQ(process_id, event->process_id);
CHECK_GE(event->timestamp, timestamp);
timestamp = event->timestamp;
CHECK_EQ(TRACE_EVENT_PHASE_END, event->phase());
CHECK_EQ(thread_id, event->thread_id());
CHECK_EQ(process_id, event->process_id());
CHECK_GE(event->timestamp(), timestamp);
timestamp = event->timestamp();
event = harness.get_event(4);
CHECK_EQ(TRACE_EVENT_PHASE_END, event->phase);
CHECK_EQ(thread_id, event->thread_id);
CHECK_EQ(process_id, event->process_id);
CHECK_GE(event->timestamp, timestamp);
timestamp = event->timestamp;
CHECK_EQ(TRACE_EVENT_PHASE_END, event->phase());
CHECK_EQ(thread_id, event->thread_id());
CHECK_EQ(process_id, event->process_id());
CHECK_GE(event->timestamp(), timestamp);
timestamp = event->timestamp();
event = harness.get_event(5);
CHECK_EQ(TRACE_EVENT_PHASE_END, event->phase);
CHECK_EQ(thread_id, event->thread_id);
CHECK_EQ(process_id, event->process_id);
CHECK_GE(event->timestamp, timestamp);
timestamp = event->timestamp;
CHECK_EQ(TRACE_EVENT_PHASE_END, event->phase());
CHECK_EQ(thread_id, event->thread_id());
CHECK_EQ(process_id, event->process_id());
CHECK_GE(event->timestamp(), timestamp);
timestamp = event->timestamp();
CHECK_EQ(6, harness.events_size());
}
// Replacement for 'TestTracingController'
TEST(Categories) {
TracingTestHarness harness;
harness.StartTracing();
{
TRACE_EVENT0("v8", "v8.Test");
// cat category is not included in default config
TRACE_EVENT0("cat", "v8.Test2");
TRACE_EVENT0("v8", "v8.Test3");
}
TRACE_EVENT_INSTANT0("v8", "final event not captured",
TRACE_EVENT_SCOPE_THREAD);
harness.StopTracing();
CHECK_EQ(4, harness.events_size());
TraceEvent* event = harness.get_event(0);
CHECK_EQ("v8.Test", event->name());
event = harness.get_event(1);
CHECK_EQ("v8.Test3", event->name());
}
// Replacement for 'TestTracingControllerMultipleArgsAndCopy'
TEST(MultipleArgsAndCopy) {
uint64_t aa = 11;
unsigned int bb = 22;
uint16_t cc = 33;
unsigned char dd = 44;
int64_t ee = -55;
int ff = -66;
int16_t gg = -77;
signed char hh = -88;
bool ii1 = true;
bool ii2 = false;
double jj1 = 99.0;
double jj2 = 1e100;
double jj3 = std::numeric_limits<double>::quiet_NaN();
double jj4 = std::numeric_limits<double>::infinity();
double jj5 = -std::numeric_limits<double>::infinity();
void* kk = &aa;
const char* ll = "100";
std::string mm = "INIT";
std::string mmm = "\"INIT\"";
TracingTestHarness harness;
harness.StartTracing();
// Create a scope for the tracing controller to terminate the trace writer.
{
TRACE_EVENT1("v8", "v8.Test.aa", "aa", aa);
TRACE_EVENT1("v8", "v8.Test.bb", "bb", bb);
TRACE_EVENT1("v8", "v8.Test.cc", "cc", cc);
TRACE_EVENT1("v8", "v8.Test.dd", "dd", dd);
TRACE_EVENT1("v8", "v8.Test.ee", "ee", ee);
TRACE_EVENT1("v8", "v8.Test.ff", "ff", ff);
TRACE_EVENT1("v8", "v8.Test.gg", "gg", gg);
TRACE_EVENT1("v8", "v8.Test.hh", "hh", hh);
TRACE_EVENT1("v8", "v8.Test.ii", "ii1", ii1);
TRACE_EVENT1("v8", "v8.Test.ii", "ii2", ii2);
TRACE_EVENT1("v8", "v8.Test.jj1", "jj1", jj1);
TRACE_EVENT1("v8", "v8.Test.jj2", "jj2", jj2);
TRACE_EVENT1("v8", "v8.Test.jj3", "jj3", jj3);
TRACE_EVENT1("v8", "v8.Test.jj4", "jj4", jj4);
TRACE_EVENT1("v8", "v8.Test.jj5", "jj5", jj5);
TRACE_EVENT1("v8", "v8.Test.kk", "kk", kk);
TRACE_EVENT1("v8", "v8.Test.ll", "ll", ll);
TRACE_EVENT1("v8", "v8.Test.mm", "mm", TRACE_STR_COPY(mmm.c_str()));
TRACE_EVENT2("v8", "v8.Test2.1", "aa", aa, "ll", ll);
TRACE_EVENT2("v8", "v8.Test2.2", "mm1", TRACE_STR_COPY(mm.c_str()), "mm2",
TRACE_STR_COPY(mmm.c_str()));
// Check copies are correct.
TRACE_EVENT_COPY_INSTANT0("v8", mm.c_str(), TRACE_EVENT_SCOPE_THREAD);
TRACE_EVENT_COPY_INSTANT2("v8", mm.c_str(), TRACE_EVENT_SCOPE_THREAD, "mm1",
mm.c_str(), "mm2", mmm.c_str());
mm = "CHANGED";
mmm = "CHANGED";
TRACE_EVENT_INSTANT1("v8", "v8.Test", TRACE_EVENT_SCOPE_THREAD, "a1",
new ConvertableToTraceFormatMock(42));
std::unique_ptr<ConvertableToTraceFormatMock> trace_event_arg(
new ConvertableToTraceFormatMock(42));
TRACE_EVENT_INSTANT2("v8", "v8.Test", TRACE_EVENT_SCOPE_THREAD, "a1",
std::move(trace_event_arg), "a2",
new ConvertableToTraceFormatMock(123));
}
TRACE_EVENT_INSTANT0("v8", "final event not captured",
TRACE_EVENT_SCOPE_THREAD);
harness.StopTracing();
// 20 START/END events, 4 INSTANT events.
CHECK_EQ(44, harness.events_size());
TraceEvent* event = harness.get_event(0);
CHECK_EQ("aa", event->args()[0].name());
CHECK_EQ(aa, event->args()[0].uint_value());
event = harness.get_event(1);
CHECK_EQ("bb", event->args()[0].name());
CHECK_EQ(bb, event->args()[0].uint_value());
event = harness.get_event(2);
CHECK_EQ("cc", event->args()[0].name());
CHECK_EQ(cc, event->args()[0].uint_value());
event = harness.get_event(3);
CHECK_EQ("dd", event->args()[0].name());
CHECK_EQ(dd, event->args()[0].uint_value());
event = harness.get_event(4);
CHECK_EQ("ee", event->args()[0].name());
CHECK_EQ(ee, event->args()[0].int_value());
event = harness.get_event(5);
CHECK_EQ("ff", event->args()[0].name());
CHECK_EQ(ff, event->args()[0].int_value());
event = harness.get_event(6);
CHECK_EQ("gg", event->args()[0].name());
CHECK_EQ(gg, event->args()[0].int_value());
event = harness.get_event(7);
CHECK_EQ("hh", event->args()[0].name());
CHECK_EQ(hh, event->args()[0].int_value());
event = harness.get_event(8);
CHECK_EQ("ii1", event->args()[0].name());
CHECK_EQ(ii1, event->args()[0].bool_value());
event = harness.get_event(9);
CHECK_EQ("ii2", event->args()[0].name());
CHECK_EQ(ii2, event->args()[0].bool_value());
event = harness.get_event(10);
CHECK_EQ("jj1", event->args()[0].name());
CHECK_EQ(jj1, event->args()[0].double_value());
event = harness.get_event(11);
CHECK_EQ("jj2", event->args()[0].name());
CHECK_EQ(jj2, event->args()[0].double_value());
event = harness.get_event(12);
CHECK_EQ("jj3", event->args()[0].name());
CHECK(std::isnan(event->args()[0].double_value()));
event = harness.get_event(13);
CHECK_EQ("jj4", event->args()[0].name());
CHECK_EQ(jj4, event->args()[0].double_value());
event = harness.get_event(14);
CHECK_EQ("jj5", event->args()[0].name());
CHECK_EQ(jj5, event->args()[0].double_value());
event = harness.get_event(15);
CHECK_EQ("kk", event->args()[0].name());
CHECK_EQ(kk, reinterpret_cast<void*>(event->args()[0].pointer_value()));
event = harness.get_event(16);
CHECK_EQ("ll", event->args()[0].name());
CHECK_EQ(ll, event->args()[0].string_value());
event = harness.get_event(17);
CHECK_EQ("mm", event->args()[0].name());
CHECK_EQ("\"INIT\"", event->args()[0].string_value());
event = harness.get_event(18);
CHECK_EQ("v8.Test2.1", event->name());
CHECK_EQ("aa", event->args()[0].name());
CHECK_EQ(aa, event->args()[0].uint_value());
CHECK_EQ("ll", event->args()[1].name());
CHECK_EQ(ll, event->args()[1].string_value());
event = harness.get_event(19);
CHECK_EQ("mm1", event->args()[0].name());
CHECK_EQ("INIT", event->args()[0].string_value());
CHECK_EQ("mm2", event->args()[1].name());
CHECK_EQ("\"INIT\"", event->args()[1].string_value());
event = harness.get_event(20);
CHECK_EQ("INIT", event->name());
event = harness.get_event(21);
CHECK_EQ("INIT", event->name());
CHECK_EQ("mm1", event->args()[0].name());
CHECK_EQ("INIT", event->args()[0].string_value());
CHECK_EQ("mm2", event->args()[1].name());
CHECK_EQ("\"INIT\"", event->args()[1].string_value());
event = harness.get_event(22);
CHECK_EQ("a1", event->args()[0].name());
CHECK_EQ("[42,42]", event->args()[0].json_value());
event = harness.get_event(23);
CHECK_EQ("a1", event->args()[0].name());
CHECK_EQ("[42,42]", event->args()[0].json_value());
CHECK_EQ("a2", event->args()[1].name());
CHECK_EQ("[123,123]", event->args()[1].json_value());
}
TEST(JsonIntegrationTest) {
// Check that tricky values are rendered correctly in the JSON output.
double big_num = 1e100;
double nan_num = std::numeric_limits<double>::quiet_NaN();
double inf_num = std::numeric_limits<double>::infinity();
double neg_inf_num = -std::numeric_limits<double>::infinity();
TracingTestHarness harness;
harness.StartTracing();
{
TRACE_EVENT1("v8", "v8.Test.1", "1", big_num);
TRACE_EVENT1("v8", "v8.Test.2", "2", nan_num);
TRACE_EVENT1("v8", "v8.Test.3", "3", inf_num);
TRACE_EVENT1("v8", "v8.Test.4", "4", neg_inf_num);
}
TRACE_EVENT_INSTANT0("v8", "final event not captured",
TRACE_EVENT_SCOPE_THREAD);
harness.StopTracing();
std::string json = harness.perfetto_json_stream();
std::vector<std::string> all_args;
GetJSONStrings(&all_args, json, "\"args\"", "{", "}");
CHECK_EQ("\"1\":1e+100", all_args[0]);
CHECK_EQ("\"2\":\"NaN\"", all_args[1]);
CHECK_EQ("\"3\":\"Infinity\"", all_args[2]);
CHECK_EQ("\"4\":\"-Infinity\"", all_args[3]);
}
TEST(TracingPerfetto) {
::perfetto::TraceConfig perfetto_trace_config;
perfetto_trace_config.add_buffers()->set_size_kb(4096);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment