[PATCH] D39086: Performance tracing facility for clangd.
kadircet added inline comments.
Comment at: clang-tools-extra/trunk/unittests/clangd/TraceTests.cpp:57
+}
+std::string VS = V->getValue(Tmp).str();
+if (VS != I->second) {
RKSimon wrote:
> @sammccall PVS Studio is reporting a potential null dereference here:
> https://www.viva64.com/en/b/0771/ (Snippet 8)
Sent out
https://github.com/llvm/llvm-project/commit/68b48339e5b2e7c5f9b74e790b4a4e2419a0a50b
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D39086/new/
https://reviews.llvm.org/D39086
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39086: Performance tracing facility for clangd.
RKSimon added inline comments.
Herald added subscribers: usaxena95, kadircet, arphaman, MaskRay.
Comment at: clang-tools-extra/trunk/unittests/clangd/TraceTests.cpp:57
+}
+std::string VS = V->getValue(Tmp).str();
+if (VS != I->second) {
@sammccall PVS Studio is reporting a potential null dereference here:
https://www.viva64.com/en/b/0771/ (Snippet 8)
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D39086/new/
https://reviews.llvm.org/D39086
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39086: Performance tracing facility for clangd.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL317193: Performance tracing facility for clangd. (authored
by sammccall).
Changed prior to commit:
https://reviews.llvm.org/D39086?vs=121253=121257#toc
Repository:
rL LLVM
https://reviews.llvm.org/D39086
Files:
clang-tools-extra/trunk/clangd/CMakeLists.txt
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdUnit.cpp
clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp
clang-tools-extra/trunk/clangd/ProtocolHandlers.cpp
clang-tools-extra/trunk/clangd/Trace.cpp
clang-tools-extra/trunk/clangd/Trace.h
clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
clang-tools-extra/trunk/test/clangd/trace.test
clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
clang-tools-extra/trunk/unittests/clangd/TraceTests.cpp
Index: clang-tools-extra/trunk/test/clangd/trace.test
===
--- clang-tools-extra/trunk/test/clangd/trace.test
+++ clang-tools-extra/trunk/test/clangd/trace.test
@@ -0,0 +1,16 @@
+# RUN: clangd -run-synchronously -trace %t < %s && FileCheck %s < %t
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+#
+Content-Length: 152
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///foo.c","languageId":"c","version":1,"text":"void main() {}"}}}
+# CHECK: "textDocument/didOpen"
+# CHECK: "Preamble: /foo.c"
+# CHECK: "Build: /foo.c"
+Content-Length: 44
+
+{"jsonrpc":"2.0","id":5,"method":"shutdown"}
Index: clang-tools-extra/trunk/unittests/clangd/TraceTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/TraceTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/TraceTests.cpp
@@ -0,0 +1,120 @@
+//===-- TraceTests.cpp - Tracing unit tests -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Trace.h"
+
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/SourceMgr.h"
+#include "llvm/Support/YAMLParser.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+using namespace llvm;
+
+MATCHER_P(StringNode, Val, "") {
+ if (arg->getType() != yaml::Node::NK_Scalar) {
+*result_listener << "is a " << arg->getVerbatimTag();
+return false;
+ }
+ SmallString<32> S;
+ return Val == static_cast(arg)->getValue(S);
+}
+
+// Checks that N is a Mapping (JS object) with the expected scalar properties.
+// The object must have all the Expected properties, but may have others.
+bool VerifyObject(yaml::Node , std::map Expected) {
+ auto *M = dyn_cast();
+ if (!M) {
+ADD_FAILURE() << "Not an object";
+return false;
+ }
+ bool Match = true;
+ SmallString<32> Tmp;
+ for (auto Prop : *M) {
+auto *K = dyn_cast_or_null(Prop.getKey());
+if (!K)
+ continue;
+std::string KS = K->getValue(Tmp).str();
+auto I = Expected.find(KS);
+if (I == Expected.end())
+ continue; // Ignore properties with no assertion.
+
+auto *V = dyn_cast_or_null(Prop.getValue());
+if (!V) {
+ ADD_FAILURE() << KS << " is not a string";
+ Match = false;
+}
+std::string VS = V->getValue(Tmp).str();
+if (VS != I->second) {
+ ADD_FAILURE() << KS << " expected " << I->second << " but actual " << VS;
+ Match = false;
+}
+Expected.erase(I);
+ }
+ for (const auto : Expected) {
+ADD_FAILURE() << P.first << " missing, expected " << P.second;
+Match = false;
+ }
+ return Match;
+}
+
+TEST(TraceTest, SmokeTest) {
+ // Capture some events.
+ std::string JSON;
+ {
+raw_string_ostream OS(JSON);
+auto Session = trace::Session::create(OS);
+{
+ trace::Span S("A");
+ trace::log("B");
+}
+ }
+
+ // Get the root JSON object using the YAML parser.
+ SourceMgr SM;
+ yaml::Stream Stream(JSON, SM);
+ auto Doc = Stream.begin();
+ ASSERT_NE(Doc, Stream.end());
+ auto *Root = dyn_cast_or_null(Doc->getRoot());
+ ASSERT_NE(Root, nullptr) << "Root should be an object";
+
+ // We expect in order:
+ // displayTimeUnit: "ns"
+ // traceEvents: [process name, thread name, start span, log, end span]
+ // (The order doesn't matter, but the YAML parser is awkward to use otherwise)
+ auto Prop = Root->begin();
+ ASSERT_NE(Prop, Root->end()) << "Expected displayTimeUnit property";
+ ASSERT_THAT(Prop->getKey(), StringNode("displayTimeUnit"));
+ EXPECT_THAT(Prop->getValue(), StringNode("ns"));
[PATCH] D39086: Performance tracing facility for clangd.
sammccall updated this revision to Diff 121253.
sammccall added a comment.
clang-format
https://reviews.llvm.org/D39086
Files:
clangd/CMakeLists.txt
clangd/ClangdServer.cpp
clangd/ClangdUnit.cpp
clangd/JSONRPCDispatcher.cpp
clangd/ProtocolHandlers.cpp
clangd/Trace.cpp
clangd/Trace.h
clangd/tool/ClangdMain.cpp
test/clangd/trace.test
unittests/clangd/CMakeLists.txt
unittests/clangd/TraceTests.cpp
Index: unittests/clangd/TraceTests.cpp
===
--- /dev/null
+++ unittests/clangd/TraceTests.cpp
@@ -0,0 +1,119 @@
+//===-- TraceTests.cpp - Tracing unit tests -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Trace.h"
+
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/SourceMgr.h"
+#include "llvm/Support/YAMLParser.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+using namespace llvm;
+
+MATCHER_P(StringNode, Val, "") {
+ if (arg->getType() != yaml::Node::NK_Scalar) {
+*result_listener << "is a " << arg->getVerbatimTag();
+return false;
+ }
+ SmallString<32> S;
+ return Val == static_cast(arg)->getValue(S);
+}
+
+// Checks that N is a Mapping (JS object) with the expected scalar properties.
+// The object must have all the Expected properties, but may have others.
+bool VerifyObject(yaml::Node , std::map Expected) {
+ auto *M = dyn_cast();
+ if (!M) {
+ADD_FAILURE() << "Not an object";
+return false;
+ }
+ bool Match = true;
+ SmallString<32> Tmp;
+ for (auto Prop : *M) {
+auto *K = dyn_cast_or_null(Prop.getKey());
+if (!K)
+ continue;
+std::string KS = K->getValue(Tmp).str();
+auto I = Expected.find(KS);
+if (I == Expected.end())
+ continue; // Ignore properties with no assertion.
+
+auto *V = dyn_cast_or_null(Prop.getValue());
+if (!V) {
+ ADD_FAILURE() << KS << " is not a string";
+ Match = false;
+}
+std::string VS = V->getValue(Tmp).str();
+if (VS != I->second) {
+ ADD_FAILURE() << KS << " expected " << I->second << " but actual " << VS;
+ Match = false;
+}
+Expected.erase(I);
+ }
+ for (const auto : Expected) {
+ADD_FAILURE() << P.first << " missing, expected " << P.second;
+Match = false;
+ }
+ return Match;
+}
+
+TEST(TraceTest, SmokeTest) {
+ // Capture some events.
+ std::string JSON;
+ raw_string_ostream OS(JSON);
+ trace::start(OS);
+ {
+trace::Span S("A");
+trace::log("B");
+ }
+ trace::stop();
+
+ // Get the root JSON object using the YAML parser.
+ SourceMgr SM;
+ yaml::Stream Stream(JSON, SM);
+ auto Doc = Stream.begin();
+ ASSERT_NE(Doc, Stream.end());
+ auto *Root = dyn_cast_or_null(Doc->getRoot());
+ ASSERT_NE(Root, nullptr) << "Root should be an object";
+
+ // We expect in order:
+ // displayTimeUnit: "ns"
+ // traceEvents: [process name, thread name, start span, log, end span]
+ // (The order doesn't matter, but the YAML parser is awkward to use otherwise)
+ auto Prop = Root->begin();
+ ASSERT_NE(Prop, Root->end()) << "Expected displayTimeUnit property";
+ ASSERT_THAT(Prop->getKey(), StringNode("displayTimeUnit"));
+ EXPECT_THAT(Prop->getValue(), StringNode("ns"));
+ ASSERT_NE(++Prop, Root->end()) << "Expected traceEvents property";
+ EXPECT_THAT(Prop->getKey(), StringNode("traceEvents"));
+ auto *Events = dyn_cast_or_null(Prop->getValue());
+ ASSERT_NE(Events, nullptr) << "traceEvents should be an array";
+ auto Event = Events->begin();
+ ASSERT_NE(Event, Events->end()) << "Expected process name";
+ EXPECT_TRUE(VerifyObject(*Event, {{"ph", "M"}, {"name", "process_name"}}));
+ ASSERT_NE(++Event, Events->end()) << "Expected thread name";
+ EXPECT_TRUE(VerifyObject(*Event, {{"ph", "M"}, {"name", "thread_name"}}));
+ ASSERT_NE(++Event, Events->end()) << "Expected span start";
+ EXPECT_TRUE(VerifyObject(*Event, {{"ph", "B"}, {"name", "A"}}));
+ ASSERT_NE(++Event, Events->end()) << "Expected log message";
+ EXPECT_TRUE(VerifyObject(*Event, {{"ph", "i"}, {"name", "B"}}));
+ ASSERT_NE(++Event, Events->end()) << "Expected span end";
+ EXPECT_TRUE(VerifyObject(*Event, {{"ph", "E"}}));
+ ASSERT_EQ(++Event, Events->end());
+ ASSERT_EQ(++Prop, Root->end());
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -10,6 +10,7 @@
add_extra_unittest(ClangdTests
ClangdTests.cpp
+ TraceTests.cpp
)
target_link_libraries(ClangdTests
Index: test/clangd/trace.test
[PATCH] D39086: Performance tracing facility for clangd.
sammccall added inline comments. Comment at: clangd/Trace.h:38 + // Starts a sessions capturing trace events and writing Trace Event JSON. + static std::unique_ptr createJSON(llvm::raw_ostream ); + ~Session(); ioeric wrote: > `createJSON` is a bit confusing... maybe just `create` since json is just the > underlying representation which users don't have to know about? I think later we might have an API for non-JSON sessions (crazy google internal tracing tools) but until then, `create` is a fine name. https://reviews.llvm.org/D39086 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39086: Performance tracing facility for clangd.
sammccall updated this revision to Diff 121252.
sammccall marked an inline comment as done.
sammccall added a comment.
createJSON -> create
https://reviews.llvm.org/D39086
Files:
clangd/CMakeLists.txt
clangd/ClangdServer.cpp
clangd/ClangdUnit.cpp
clangd/JSONRPCDispatcher.cpp
clangd/ProtocolHandlers.cpp
clangd/Trace.cpp
clangd/Trace.h
clangd/tool/ClangdMain.cpp
test/clangd/trace.test
unittests/clangd/CMakeLists.txt
unittests/clangd/TraceTests.cpp
Index: unittests/clangd/TraceTests.cpp
===
--- /dev/null
+++ unittests/clangd/TraceTests.cpp
@@ -0,0 +1,117 @@
+//===-- TraceTests.cpp - Tracing unit tests -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Trace.h"
+
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/SourceMgr.h"
+#include "llvm/Support/YAMLParser.h"
+#include "gtest/gtest.h"
+#include "gmock/gmock.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+using namespace llvm;
+
+MATCHER_P(StringNode, Val, "") {
+ if (arg->getType() != yaml::Node::NK_Scalar) {
+*result_listener << "is a " << arg->getVerbatimTag();
+return false;
+ }
+ SmallString<32> S;
+ return Val == static_cast(arg)->getValue(S);
+}
+
+// Checks that N is a Mapping (JS object) with the expected scalar properties.
+// The object must have all the Expected properties, but may have others.
+bool VerifyObject(yaml::Node , std::map Expected) {
+ auto* M = dyn_cast();
+ if (!M) {
+ADD_FAILURE() << "Not an object";
+return false;
+ }
+ bool Match = true;
+ SmallString<32> Tmp;
+ for (auto Prop : *M) {
+auto* K = dyn_cast_or_null(Prop.getKey());
+if (!K) continue;
+std::string KS = K->getValue(Tmp).str();
+auto I = Expected.find(KS);
+if (I == Expected.end()) continue; // Ignore properties with no assertion.
+
+auto* V = dyn_cast_or_null(Prop.getValue());
+if (!V) {
+ ADD_FAILURE() << KS << " is not a string";
+ Match = false;
+}
+std::string VS = V->getValue(Tmp).str();
+if (VS != I->second) {
+ ADD_FAILURE() << KS << " expected " << I->second << " but actual " << VS;
+ Match = false;
+}
+Expected.erase(I);
+ }
+ for (const auto& P : Expected) {
+ADD_FAILURE() << P.first << " missing, expected " << P.second;
+Match = false;
+ }
+ return Match;
+}
+
+TEST(TraceTest, SmokeTest) {
+ // Capture some events.
+ std::string JSON;
+ raw_string_ostream OS(JSON);
+ trace::start(OS);
+ {
+trace::Span S("A");
+trace::log("B");
+ }
+ trace::stop();
+
+ // Get the root JSON object using the YAML parser.
+ SourceMgr SM;
+ yaml::Stream Stream(JSON, SM);
+ auto Doc = Stream.begin();
+ ASSERT_NE(Doc, Stream.end());
+ auto* Root = dyn_cast_or_null(Doc->getRoot());
+ ASSERT_NE(Root, nullptr) << "Root should be an object";
+
+ // We expect in order:
+ // displayTimeUnit: "ns"
+ // traceEvents: [process name, thread name, start span, log, end span]
+ // (The order doesn't matter, but the YAML parser is awkward to use otherwise)
+ auto Prop = Root->begin();
+ ASSERT_NE(Prop, Root->end()) << "Expected displayTimeUnit property";
+ ASSERT_THAT(Prop->getKey(), StringNode("displayTimeUnit"));
+ EXPECT_THAT(Prop->getValue(), StringNode("ns"));
+ ASSERT_NE(++Prop, Root->end()) << "Expected traceEvents property";
+ EXPECT_THAT(Prop->getKey(), StringNode("traceEvents"));
+ auto* Events = dyn_cast_or_null(Prop->getValue());
+ ASSERT_NE(Events, nullptr) << "traceEvents should be an array";
+ auto Event = Events->begin();
+ ASSERT_NE(Event, Events->end()) << "Expected process name";
+ EXPECT_TRUE(VerifyObject(*Event, {{"ph", "M"}, {"name", "process_name"}}));
+ ASSERT_NE(++Event, Events->end()) << "Expected thread name";
+ EXPECT_TRUE(VerifyObject(*Event, {{"ph", "M"}, {"name", "thread_name"}}));
+ ASSERT_NE(++Event, Events->end()) << "Expected span start";
+ EXPECT_TRUE(VerifyObject(*Event, {{"ph", "B"}, {"name", "A"}}));
+ ASSERT_NE(++Event, Events->end()) << "Expected log message";
+ EXPECT_TRUE(VerifyObject(*Event, {{"ph", "i"}, {"name", "B"}}));
+ ASSERT_NE(++Event, Events->end()) << "Expected span end";
+ EXPECT_TRUE(VerifyObject(*Event, {{"ph", "E"}}));
+ ASSERT_EQ(++Event, Events->end());
+ ASSERT_EQ(++Prop, Root->end());
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -10,6 +10,7 @@
add_extra_unittest(ClangdTests
ClangdTests.cpp
+ TraceTests.cpp
)
[PATCH] D39086: Performance tracing facility for clangd.
ioeric accepted this revision. ioeric added a comment. still lgtm Comment at: clangd/Trace.h:38 + // Starts a sessions capturing trace events and writing Trace Event JSON. + static std::unique_ptr createJSON(llvm::raw_ostream ); + ~Session(); `createJSON` is a bit confusing... maybe just `create` since json is just the underlying representation which users don't have to know about? https://reviews.llvm.org/D39086 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39086: Performance tracing facility for clangd.
sammccall updated this revision to Diff 121247.
sammccall added a comment.
Provide RAII-like interface to trace functionality.
Note that we may want to provide a backend API here.
https://reviews.llvm.org/D39086
Files:
clangd/CMakeLists.txt
clangd/ClangdServer.cpp
clangd/ClangdUnit.cpp
clangd/JSONRPCDispatcher.cpp
clangd/ProtocolHandlers.cpp
clangd/Trace.cpp
clangd/Trace.h
clangd/tool/ClangdMain.cpp
test/clangd/trace.test
unittests/clangd/CMakeLists.txt
unittests/clangd/TraceTests.cpp
Index: unittests/clangd/TraceTests.cpp
===
--- /dev/null
+++ unittests/clangd/TraceTests.cpp
@@ -0,0 +1,117 @@
+//===-- TraceTests.cpp - Tracing unit tests -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Trace.h"
+
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/SourceMgr.h"
+#include "llvm/Support/YAMLParser.h"
+#include "gtest/gtest.h"
+#include "gmock/gmock.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+using namespace llvm;
+
+MATCHER_P(StringNode, Val, "") {
+ if (arg->getType() != yaml::Node::NK_Scalar) {
+*result_listener << "is a " << arg->getVerbatimTag();
+return false;
+ }
+ SmallString<32> S;
+ return Val == static_cast(arg)->getValue(S);
+}
+
+// Checks that N is a Mapping (JS object) with the expected scalar properties.
+// The object must have all the Expected properties, but may have others.
+bool VerifyObject(yaml::Node , std::map Expected) {
+ auto* M = dyn_cast();
+ if (!M) {
+ADD_FAILURE() << "Not an object";
+return false;
+ }
+ bool Match = true;
+ SmallString<32> Tmp;
+ for (auto Prop : *M) {
+auto* K = dyn_cast_or_null(Prop.getKey());
+if (!K) continue;
+std::string KS = K->getValue(Tmp).str();
+auto I = Expected.find(KS);
+if (I == Expected.end()) continue; // Ignore properties with no assertion.
+
+auto* V = dyn_cast_or_null(Prop.getValue());
+if (!V) {
+ ADD_FAILURE() << KS << " is not a string";
+ Match = false;
+}
+std::string VS = V->getValue(Tmp).str();
+if (VS != I->second) {
+ ADD_FAILURE() << KS << " expected " << I->second << " but actual " << VS;
+ Match = false;
+}
+Expected.erase(I);
+ }
+ for (const auto& P : Expected) {
+ADD_FAILURE() << P.first << " missing, expected " << P.second;
+Match = false;
+ }
+ return Match;
+}
+
+TEST(TraceTest, SmokeTest) {
+ // Capture some events.
+ std::string JSON;
+ raw_string_ostream OS(JSON);
+ trace::start(OS);
+ {
+trace::Span S("A");
+trace::log("B");
+ }
+ trace::stop();
+
+ // Get the root JSON object using the YAML parser.
+ SourceMgr SM;
+ yaml::Stream Stream(JSON, SM);
+ auto Doc = Stream.begin();
+ ASSERT_NE(Doc, Stream.end());
+ auto* Root = dyn_cast_or_null(Doc->getRoot());
+ ASSERT_NE(Root, nullptr) << "Root should be an object";
+
+ // We expect in order:
+ // displayTimeUnit: "ns"
+ // traceEvents: [process name, thread name, start span, log, end span]
+ // (The order doesn't matter, but the YAML parser is awkward to use otherwise)
+ auto Prop = Root->begin();
+ ASSERT_NE(Prop, Root->end()) << "Expected displayTimeUnit property";
+ ASSERT_THAT(Prop->getKey(), StringNode("displayTimeUnit"));
+ EXPECT_THAT(Prop->getValue(), StringNode("ns"));
+ ASSERT_NE(++Prop, Root->end()) << "Expected traceEvents property";
+ EXPECT_THAT(Prop->getKey(), StringNode("traceEvents"));
+ auto* Events = dyn_cast_or_null(Prop->getValue());
+ ASSERT_NE(Events, nullptr) << "traceEvents should be an array";
+ auto Event = Events->begin();
+ ASSERT_NE(Event, Events->end()) << "Expected process name";
+ EXPECT_TRUE(VerifyObject(*Event, {{"ph", "M"}, {"name", "process_name"}}));
+ ASSERT_NE(++Event, Events->end()) << "Expected thread name";
+ EXPECT_TRUE(VerifyObject(*Event, {{"ph", "M"}, {"name", "thread_name"}}));
+ ASSERT_NE(++Event, Events->end()) << "Expected span start";
+ EXPECT_TRUE(VerifyObject(*Event, {{"ph", "B"}, {"name", "A"}}));
+ ASSERT_NE(++Event, Events->end()) << "Expected log message";
+ EXPECT_TRUE(VerifyObject(*Event, {{"ph", "i"}, {"name", "B"}}));
+ ASSERT_NE(++Event, Events->end()) << "Expected span end";
+ EXPECT_TRUE(VerifyObject(*Event, {{"ph", "E"}}));
+ ASSERT_EQ(++Event, Events->end());
+ ASSERT_EQ(++Prop, Root->end());
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -10,6 +10,7 @@
add_extra_unittest(ClangdTests
ClangdTests.cpp
[PATCH] D39086: Performance tracing facility for clangd.
sammccall marked an inline comment as done.
sammccall added inline comments.
Comment at: clangd/Trace.cpp:87
+
+static Tracer* T = nullptr;
+} // namespace
ilya-biryukov wrote:
> Maybe use `unique_ptr` (or even `llvm::Optional`)? Otherwise
> we leak memory and don't flush the stream if users of tracing API forget to
> call `stop()`. (I think I'm on the same page with @ioeric here about
> forgetting a call to `stop()`).
>
> Having a static global of non-pod type is a no-go, but we could use static
> local variables:
>
> ```
> llvm::Optional& getTracerInstance() {
> static llvm::Optional Tracer;
> return Tracer;
> }
> ```
So Eric's solution of a scoped object that would be owned by main() makes sense
to me, and I'm happy to add it (though personally not sure it carries its
weight).
However I don't understand *your* suggestion :-)
> Having a static global of non-pod type is a no-go, but we could use static
> local variables:
> llvm::Optional& getTracerInstance() {
> static llvm::Optional Tracer;
> return Tracer;
> }
So it's not global, which means we're not calling Optional() on startup, which
would actually be totally safe (I guess the constructor should be marked as
`constexpr`).
But we *are* calling `~Optional()` and therefore `~Tracer()` at shutdown. And
maybe `~Tracer()` calls something that logs, and llvm::errs() has already been
destroyed... *this* is why avoid static objects, right?
> Otherwise we leak memory and don't flush the stream if users of tracing API
> forget to call stop()
The intended time to call `stop()` is at the end of the program. If you forget
to call it, we won't be leaking memory :-) We indeed won't flush the stream,
but that wouldn't be safe (see above).
The other option is the user is calling `start()` and `stop()` multiple times,
and they call `start()` twice without stopping. We already assert on this, and
anyone who doesn't reproducibly hit that assert is well into racy UB-land
(logging methods may be called concurrently with each other, but everything
else must be synchronized).
Overall I'm not really seeing what scenario we can detect and safely do
something about.
https://reviews.llvm.org/D39086
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39086: Performance tracing facility for clangd.
sammccall updated this revision to Diff 119864.
sammccall added a comment.
Address FIXME now that r316330 is in.
https://reviews.llvm.org/D39086
Files:
clangd/CMakeLists.txt
clangd/ClangdServer.cpp
clangd/ClangdUnit.cpp
clangd/JSONRPCDispatcher.cpp
clangd/ProtocolHandlers.cpp
clangd/Trace.cpp
clangd/Trace.h
clangd/tool/ClangdMain.cpp
test/clangd/trace.test
unittests/clangd/CMakeLists.txt
unittests/clangd/TraceTests.cpp
Index: unittests/clangd/TraceTests.cpp
===
--- /dev/null
+++ unittests/clangd/TraceTests.cpp
@@ -0,0 +1,117 @@
+//===-- TraceTests.cpp - Tracing unit tests -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Trace.h"
+
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/SourceMgr.h"
+#include "llvm/Support/YAMLParser.h"
+#include "gtest/gtest.h"
+#include "gmock/gmock.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+using namespace llvm;
+
+MATCHER_P(StringNode, Val, "") {
+ if (arg->getType() != yaml::Node::NK_Scalar) {
+*result_listener << "is a " << arg->getVerbatimTag();
+return false;
+ }
+ SmallString<32> S;
+ return Val == static_cast(arg)->getValue(S);
+}
+
+// Checks that N is a Mapping (JS object) with the expected scalar properties.
+// The object must have all the Expected properties, but may have others.
+bool VerifyObject(yaml::Node , std::map Expected) {
+ auto* M = dyn_cast();
+ if (!M) {
+ADD_FAILURE() << "Not an object";
+return false;
+ }
+ bool Match = true;
+ SmallString<32> Tmp;
+ for (auto Prop : *M) {
+auto* K = dyn_cast_or_null(Prop.getKey());
+if (!K) continue;
+std::string KS = K->getValue(Tmp).str();
+auto I = Expected.find(KS);
+if (I == Expected.end()) continue; // Ignore properties with no assertion.
+
+auto* V = dyn_cast_or_null(Prop.getValue());
+if (!V) {
+ ADD_FAILURE() << KS << " is not a string";
+ Match = false;
+}
+std::string VS = V->getValue(Tmp).str();
+if (VS != I->second) {
+ ADD_FAILURE() << KS << " expected " << I->second << " but actual " << VS;
+ Match = false;
+}
+Expected.erase(I);
+ }
+ for (const auto& P : Expected) {
+ADD_FAILURE() << P.first << " missing, expected " << P.second;
+Match = false;
+ }
+ return Match;
+}
+
+TEST(TraceTest, SmokeTest) {
+ // Capture some events.
+ std::string JSON;
+ raw_string_ostream OS(JSON);
+ trace::start(OS);
+ {
+trace::Span S("A");
+trace::log("B");
+ }
+ trace::stop();
+
+ // Get the root JSON object using the YAML parser.
+ SourceMgr SM;
+ yaml::Stream Stream(JSON, SM);
+ auto Doc = Stream.begin();
+ ASSERT_NE(Doc, Stream.end());
+ auto* Root = dyn_cast_or_null(Doc->getRoot());
+ ASSERT_NE(Root, nullptr) << "Root should be an object";
+
+ // We expect in order:
+ // displayTimeUnit: "ns"
+ // traceEvents: [process name, thread name, start span, log, end span]
+ // (The order doesn't matter, but the YAML parser is awkward to use otherwise)
+ auto Prop = Root->begin();
+ ASSERT_NE(Prop, Root->end()) << "Expected displayTimeUnit property";
+ ASSERT_THAT(Prop->getKey(), StringNode("displayTimeUnit"));
+ EXPECT_THAT(Prop->getValue(), StringNode("ns"));
+ ASSERT_NE(++Prop, Root->end()) << "Expected traceEvents property";
+ EXPECT_THAT(Prop->getKey(), StringNode("traceEvents"));
+ auto* Events = dyn_cast_or_null(Prop->getValue());
+ ASSERT_NE(Events, nullptr) << "traceEvents should be an array";
+ auto Event = Events->begin();
+ ASSERT_NE(Event, Events->end()) << "Expected process name";
+ EXPECT_TRUE(VerifyObject(*Event, {{"ph", "M"}, {"name", "process_name"}}));
+ ASSERT_NE(++Event, Events->end()) << "Expected thread name";
+ EXPECT_TRUE(VerifyObject(*Event, {{"ph", "M"}, {"name", "thread_name"}}));
+ ASSERT_NE(++Event, Events->end()) << "Expected span start";
+ EXPECT_TRUE(VerifyObject(*Event, {{"ph", "B"}, {"name", "A"}}));
+ ASSERT_NE(++Event, Events->end()) << "Expected log message";
+ EXPECT_TRUE(VerifyObject(*Event, {{"ph", "i"}, {"name", "B"}}));
+ ASSERT_NE(++Event, Events->end()) << "Expected span end";
+ EXPECT_TRUE(VerifyObject(*Event, {{"ph", "E"}}));
+ ASSERT_EQ(++Event, Events->end());
+ ASSERT_EQ(++Prop, Root->end());
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -10,6 +10,7 @@
add_extra_unittest(ClangdTests
ClangdTests.cpp
+ TraceTests.cpp
)
target_link_libraries(ClangdTests
Index:
[PATCH] D39086: Performance tracing facility for clangd.
ilya-biryukov added inline comments.
Comment at: clangd/Trace.cpp:87
+
+static Tracer* T = nullptr;
+} // namespace
Maybe use `unique_ptr` (or even `llvm::Optional`)? Otherwise we
leak memory and don't flush the stream if users of tracing API forget to call
`stop()`. (I think I'm on the same page with @ioeric here about forgetting a
call to `stop()`).
Having a static global of non-pod type is a no-go, but we could use static
local variables:
```
llvm::Optional& getTracerInstance() {
static llvm::Optional Tracer;
return Tracer;
}
```
Comment at: clangd/Trace.cpp:102
+ if (!T) return;
+ // TODO: remove str() once formatv_object is formatv-able.
+ T->event("i", formatv(R"("name":"{0}")", yaml::escape(Message.str())).str());
NIT: Use `FIXME` instead of `TODO`
https://reviews.llvm.org/D39086
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39086: Performance tracing facility for clangd.
ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. lgtm https://reviews.llvm.org/D39086 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39086: Performance tracing facility for clangd.
sammccall updated this revision to Diff 119562.
sammccall marked 3 inline comments as done.
sammccall added a comment.
Address review comments.
https://reviews.llvm.org/D39086
Files:
clangd/CMakeLists.txt
clangd/ClangdServer.cpp
clangd/ClangdUnit.cpp
clangd/JSONRPCDispatcher.cpp
clangd/ProtocolHandlers.cpp
clangd/Trace.cpp
clangd/Trace.h
clangd/tool/ClangdMain.cpp
test/clangd/trace.test
unittests/clangd/CMakeLists.txt
unittests/clangd/TraceTests.cpp
Index: unittests/clangd/TraceTests.cpp
===
--- /dev/null
+++ unittests/clangd/TraceTests.cpp
@@ -0,0 +1,117 @@
+//===-- TraceTests.cpp - Tracing unit tests -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Trace.h"
+
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/SourceMgr.h"
+#include "llvm/Support/YAMLParser.h"
+#include "gtest/gtest.h"
+#include "gmock/gmock.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+using namespace llvm;
+
+MATCHER_P(StringNode, Val, "") {
+ if (arg->getType() != yaml::Node::NK_Scalar) {
+*result_listener << "is a " << arg->getVerbatimTag();
+return false;
+ }
+ SmallString<32> S;
+ return Val == static_cast(arg)->getValue(S);
+}
+
+// Checks that N is a Mapping (JS object) with the expected scalar properties.
+// The object must have all the Expected properties, but may have others.
+bool VerifyObject(yaml::Node , std::map Expected) {
+ auto* M = dyn_cast();
+ if (!M) {
+ADD_FAILURE() << "Not an object";
+return false;
+ }
+ bool Match = true;
+ SmallString<32> Tmp;
+ for (auto Prop : *M) {
+auto* K = dyn_cast_or_null(Prop.getKey());
+if (!K) continue;
+std::string KS = K->getValue(Tmp).str();
+auto I = Expected.find(KS);
+if (I == Expected.end()) continue; // Ignore properties with no assertion.
+
+auto* V = dyn_cast_or_null(Prop.getValue());
+if (!V) {
+ ADD_FAILURE() << KS << " is not a string";
+ Match = false;
+}
+std::string VS = V->getValue(Tmp).str();
+if (VS != I->second) {
+ ADD_FAILURE() << KS << " expected " << I->second << " but actual " << VS;
+ Match = false;
+}
+Expected.erase(I);
+ }
+ for (const auto& P : Expected) {
+ADD_FAILURE() << P.first << " missing, expected " << P.second;
+Match = false;
+ }
+ return Match;
+}
+
+TEST(TraceTest, SmokeTest) {
+ // Capture some events.
+ std::string JSON;
+ raw_string_ostream OS(JSON);
+ trace::start(OS);
+ {
+trace::Span S("A");
+trace::log("B");
+ }
+ trace::stop();
+
+ // Get the root JSON object using the YAML parser.
+ SourceMgr SM;
+ yaml::Stream Stream(JSON, SM);
+ auto Doc = Stream.begin();
+ ASSERT_NE(Doc, Stream.end());
+ auto* Root = dyn_cast_or_null(Doc->getRoot());
+ ASSERT_NE(Root, nullptr) << "Root should be an object";
+
+ // We expect in order:
+ // displayTimeUnit: "ns"
+ // traceEvents: [process name, thread name, start span, log, end span]
+ // (The order doesn't matter, but the YAML parser is awkward to use otherwise)
+ auto Prop = Root->begin();
+ ASSERT_NE(Prop, Root->end()) << "Expected displayTimeUnit property";
+ ASSERT_THAT(Prop->getKey(), StringNode("displayTimeUnit"));
+ EXPECT_THAT(Prop->getValue(), StringNode("ns"));
+ ASSERT_NE(++Prop, Root->end()) << "Expected traceEvents property";
+ EXPECT_THAT(Prop->getKey(), StringNode("traceEvents"));
+ auto* Events = dyn_cast_or_null(Prop->getValue());
+ ASSERT_NE(Events, nullptr) << "traceEvents should be an array";
+ auto Event = Events->begin();
+ ASSERT_NE(Event, Events->end()) << "Expected process name";
+ EXPECT_TRUE(VerifyObject(*Event, {{"ph", "M"}, {"name", "process_name"}}));
+ ASSERT_NE(++Event, Events->end()) << "Expected thread name";
+ EXPECT_TRUE(VerifyObject(*Event, {{"ph", "M"}, {"name", "thread_name"}}));
+ ASSERT_NE(++Event, Events->end()) << "Expected span start";
+ EXPECT_TRUE(VerifyObject(*Event, {{"ph", "B"}, {"name", "A"}}));
+ ASSERT_NE(++Event, Events->end()) << "Expected log message";
+ EXPECT_TRUE(VerifyObject(*Event, {{"ph", "i"}, {"name", "B"}}));
+ ASSERT_NE(++Event, Events->end()) << "Expected span end";
+ EXPECT_TRUE(VerifyObject(*Event, {{"ph", "E"}}));
+ ASSERT_EQ(++Event, Events->end());
+ ASSERT_EQ(++Prop, Root->end());
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -10,6 +10,7 @@
add_extra_unittest(ClangdTests
ClangdTests.cpp
+ TraceTests.cpp
)
[PATCH] D39086: Performance tracing facility for clangd.
sammccall added inline comments.
Comment at: clangd/JSONRPCDispatcher.cpp:230
- // Finally, execute the action for this JSON message.
- if (!Dispatcher.call(JSONRef, Out))
-Out.log("JSON dispatch failed!\n");
+ {
+// Finally, execute the action for this JSON message.
ioeric wrote:
> Why this empty block?
Oops, was tracing here but it didn't turn out to be useful. Reverted.
Comment at: clangd/Trace.cpp:90
+
+void start(raw_ostream ) {
+ assert(!T && "start() called twice without stop()");
ioeric wrote:
> Would it make sense to have a helper class which handles starting/stopping
> in, say, constructor/destructor? ... so that we don't need to worry about
> forgetting to stop?
I didn't do this for a couple of reasons:
- it obscures the static-stateful nature of this code, making it look like
e.g. you could dynamically turn on tracing while clangd is running. (Which
would be cool, but wouldn't currently work)
- it's some more boilerplate, and isn't actually very convenient to ClangdMain
(since `start()` is conditional)
Comment at: clangd/tool/ClangdMain.cpp:98
+ llvm::errs() << "Error while opening trace file: " << EC.message();
+}
+trace::start(*TraceStream);
ioeric wrote:
> Is this intended? Start tracing even if there is an error?
Oops, thanks!
https://reviews.llvm.org/D39086
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39086: Performance tracing facility for clangd.
ioeric added a comment.
Looks good in general. A few nits.
Comment at: clangd/JSONRPCDispatcher.cpp:230
- // Finally, execute the action for this JSON message.
- if (!Dispatcher.call(JSONRef, Out))
-Out.log("JSON dispatch failed!\n");
+ {
+// Finally, execute the action for this JSON message.
Why this empty block?
Comment at: clangd/Trace.cpp:90
+
+void start(raw_ostream ) {
+ assert(!T && "start() called twice without stop()");
Would it make sense to have a helper class which handles starting/stopping in,
say, constructor/destructor? ... so that we don't need to worry about
forgetting to stop?
Comment at: clangd/tool/ClangdMain.cpp:98
+ llvm::errs() << "Error while opening trace file: " << EC.message();
+}
+trace::start(*TraceStream);
Is this intended? Start tracing even if there is an error?
Comment at: unittests/clangd/TraceTests.cpp:47
+auto I = Expected.find(KS);
+if (I == Expected.end()) continue;
+
Add a comment on why we skip unexpected keys?
https://reviews.llvm.org/D39086
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39086: Performance tracing facility for clangd.
sammccall created this revision.
Herald added a subscriber: mgorny.
This lets you visualize clangd's activity on different threads over time,
and understand critical paths of requests and object lifetimes.
The data produced can be visualized in Chrome (at chrome://tracing), or
in a standalone copy of catapult (http://github.com/catapult-project/catapult)
This patch consists of:
- a command line flag "-trace" that causes clangd to emit JSON trace data
- an API (in Trace.h) allowing clangd code to easily add events to the stream
- several initial uses of this API to capture JSON-RPC requests, builds, logs
Example result: https://photos.app.goo.gl/12L9swaz5REGQ1rm1
Caveats:
- JSON serialization is ad-hoc (isn't it everywhere?) so the API is limited to
naming events rather than attaching arbitrary metadata. I'd like to fix this (I
think we could use a JSON-object abstraction).
- The recording is very naive: events are written immediately by locking a
mutex. Contention on the mutex might disturb performance.
- For now it just traces instants or spans on the current thread. There are
other things that make sense to show (cross-thread flows, non-thread resources
such as ASTs). But we have to start somewhere.
https://reviews.llvm.org/D39086
Files:
clangd/CMakeLists.txt
clangd/ClangdServer.cpp
clangd/ClangdUnit.cpp
clangd/JSONRPCDispatcher.cpp
clangd/ProtocolHandlers.cpp
clangd/Trace.cpp
clangd/Trace.h
clangd/tool/ClangdMain.cpp
test/clangd/trace.test
unittests/clangd/CMakeLists.txt
unittests/clangd/TraceTests.cpp
Index: unittests/clangd/TraceTests.cpp
===
--- /dev/null
+++ unittests/clangd/TraceTests.cpp
@@ -0,0 +1,116 @@
+//===-- TraceTests.cpp - Tracing unit tests -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Trace.h"
+
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/SourceMgr.h"
+#include "llvm/Support/YAMLParser.h"
+#include "gtest/gtest.h"
+#include "gmock/gmock.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+using namespace llvm;
+
+MATCHER_P(StringNode, Val, "") {
+ if (arg->getType() != yaml::Node::NK_Scalar) {
+*result_listener << "is a " << arg->getVerbatimTag();
+return false;
+ }
+ SmallString<32> S;
+ return Val == static_cast(arg)->getValue(S);
+}
+
+// Checks that N is a Mapping (JS object) with the expected scalar properties.
+bool VerifyObject(yaml::Node , std::map Expected) {
+ auto* M = dyn_cast();
+ if (!M) {
+ADD_FAILURE() << "Not an object";
+return false;
+ }
+ bool Match = true;
+ SmallString<32> Tmp;
+ for (auto Prop : *M) {
+auto* K = dyn_cast_or_null(Prop.getKey());
+if (!K) continue;
+std::string KS = K->getValue(Tmp).str();
+auto I = Expected.find(KS);
+if (I == Expected.end()) continue;
+
+auto* V = dyn_cast_or_null(Prop.getValue());
+if (!V) {
+ ADD_FAILURE() << KS << " is not a string";
+ Match = false;
+}
+std::string VS = V->getValue(Tmp).str();
+if (VS != I->second) {
+ ADD_FAILURE() << KS << " expected " << I->second << " but actual " << VS;
+ Match = false;
+}
+Expected.erase(I);
+ }
+ for (const auto& P : Expected) {
+ADD_FAILURE() << P.first << " missing, expected " << P.second;
+Match = false;
+ }
+ return Match;
+}
+
+TEST(TraceTest, SmokeTest) {
+ // Capture some events.
+ std::string JSON;
+ raw_string_ostream OS(JSON);
+ trace::start(OS);
+ {
+trace::Span S("A");
+trace::log("B");
+ }
+ trace::stop();
+
+ // Get the root JSON object using the YAML parser.
+ SourceMgr SM;
+ yaml::Stream Stream(JSON, SM);
+ auto Doc = Stream.begin();
+ ASSERT_NE(Doc, Stream.end());
+ auto* Root = dyn_cast_or_null(Doc->getRoot());
+ ASSERT_NE(Root, nullptr) << "Root should be an object";
+
+ // We expect in order:
+ // displayTimeUnit: "ns"
+ // traceEvents: [process name, thread name, start span, log, end span]
+ // (The order doesn't matter, but the YAML parser is awkward to use otherwise)
+ auto Prop = Root->begin();
+ ASSERT_NE(Prop, Root->end()) << "Expected displayTimeUnit property";
+ ASSERT_THAT(Prop->getKey(), StringNode("displayTimeUnit"));
+ EXPECT_THAT(Prop->getValue(), StringNode("ns"));
+ ASSERT_NE(++Prop, Root->end()) << "Expected traceEvents property";
+ EXPECT_THAT(Prop->getKey(), StringNode("traceEvents"));
+ auto* Events = dyn_cast_or_null(Prop->getValue());
+ ASSERT_NE(Events, nullptr) << "traceEvents should be an array";
+ auto Event = Events->begin();
+ ASSERT_NE(Event, Events->end()) << "Expected process name";
+ EXPECT_TRUE(VerifyObject(*Event, {{"ph", "M"},
