[Lldb-commits] [PATCH] D107407: [rfc] target stats

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.


================
Comment at: lldb/include/lldb/Symbol/SymbolFile.h:302-305
+  virtual llvm::Optional<std::chrono::duration<double>> GetSymbolLoadingTime() {
+    return llvm::None;
+  }
+
----------------
I would add an instance variable to this base class and then return it here. Then subclasses can just access the ivar, and clients can use this accessor to access it and no need to be virtual


================
Comment at: lldb/include/lldb/Target/TargetStats.h:43
+
+  void NotifyModuleSymbolParsing(std::chrono::duration<double> duration);
+
----------------
Does this belong here? It seems like this is something we should gather at the time the stats are asked to be returned. If it does belong here, what is this saying? Total symbol parsing time? 


================
Comment at: lldb/source/API/SBTarget.cpp:213-226
 SBStructuredData SBTarget::GetStatistics() {
   LLDB_RECORD_METHOD_NO_ARGS(lldb::SBStructuredData, SBTarget, GetStatistics);
 
   SBStructuredData data;
   TargetSP target_sp(GetSP());
   if (!target_sp)
     return LLDB_RECORD_RESULT(data);
----------------
We can't replace this if this was doing something before. Was this doing nothing previously? Or are we doing everything this used to do and more?


================
Comment at: lldb/source/API/SBTarget.cpp:228-230
 void SBTarget::SetCollectingStats(bool v) {
   LLDB_RECORD_METHOD(void, SBTarget, SetCollectingStats, (bool), v);
 }
----------------
We can't change this if it did something valid prior to this diff? Or are we doing everything this used to do and more?


================
Comment at: lldb/source/API/SBTarget.cpp:234
   LLDB_RECORD_METHOD_NO_ARGS(bool, SBTarget, GetCollectingStats);
-
-  TargetSP target_sp(GetSP());
-  if (!target_sp)
-    return false;
-  return target_sp->GetCollectingStats();
+  return true;
 }
----------------
We can't change this if it did something valid prior to this diff? Or are we doing everything this used to do and more?


================
Comment at: lldb/source/Commands/CommandObjectStats.cpp:16
 
-class CommandObjectStatsEnable : public CommandObjectParsed {
-public:
----------------
We should leave this command in place. Not a good idea to remote a command, if we do, it might make some lldb command files fail in the middle or parsing them and change people's script behavior.


================
Comment at: lldb/source/Commands/CommandObjectStats.cpp:40
-
-class CommandObjectStatsDisable : public CommandObjectParsed {
 public:
----------------
We should leave this command in place. Not a good idea to remote a command, if we do, it might make some lldb command files fail in the middle or parsing them and change people's script behavior.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp:26-38
 void ManualDWARFIndex::Index() {
+  std::chrono::duration<double> t1 =
+      std::chrono::steady_clock::now().time_since_epoch();
+  DoIndex();
+  m_loading_time = std::chrono::steady_clock::now().time_since_epoch() - t1;
+}
+
----------------
Maybe use a scope C++ class to help get the time of the index here and avoid making the Index() and DoIndex() calls? The "ScopedTimer" class would be something like:

```
class ScopedTimer {
  llvm::Optional<std::chrono::duration<double>> &m_opt_time;
  std::chrono::duration<double>m_start;
public:
  ScopedTimer (llvm::Optional<std::chrono::duration<double>> &opt_time) : m_opt_time(opt_time) {
    m_start = std::chrono::steady_clock::now().time_since_epoch();
  }
  ~ScopedTimer() {
    m_opt_time = std::chrono::steady_clock::now().time_since_epoch() - m_start;
  }
}
```

And we should probably put this in a header file so it can be reused elsewhere.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.h:58
 
+  llvm::Optional<std::chrono::duration<double>> GetSymbolLoadingTime() override;
+
----------------
Remove, see comment in DWARFIndex.h


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.h:72
   void Index();
+  void DoIndex();
   void IndexUnit(DWARFUnit &unit, SymbolFileDWARFDwo *dwp, IndexSet &set);
----------------
Remove and see "ScopedTimer" comment


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.h:86
   IndexSet m_set;
+  llvm::Optional<std::chrono::duration<double>> m_loading_time;
 };
----------------
This should be in base class.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:517-522
+llvm::Optional<std::chrono::duration<double>>
+SymbolFileDWARF::GetSymbolLoadingTime() {
+  if (!m_index)
+    return llvm::None;
+  return m_index->GetSymbolLoadingTime();
+}
----------------
Remove this and just set the SymbolFile::m_index_time directly after indexing


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:323-324
 
+  llvm::Optional<std::chrono::duration<double>> GetSymbolLoadingTime() override;
+
 protected:
----------------
Remove, see comment in SymbolFile.h


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107407/new/

https://reviews.llvm.org/D107407