[Startup Tracing] The TraceConfig class [STALE]

base/trace_event/trace_event_impl.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 
 #ifndef BASE_TRACE_EVENT_TRACE_EVENT_IMPL_H_
 #define BASE_TRACE_EVENT_TRACE_EVENT_IMPL_H_
 
 #include <stack>
 #include <string>
 #include <vector>
 
 #include "base/atomicops.h"
 #include "base/base_export.h"
 #include "base/callback.h"
 #include "base/containers/hash_tables.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/memory/scoped_vector.h"
 #include "base/observer_list.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_util.h"
 #include "base/synchronization/condition_variable.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_local.h"
+#include "base/values.h"
 
 // Older style trace macros with explicit id and extra data
 // Only these macros result in publishing data to ETW as currently implemented.
 // TODO(georgesak): Update/replace these with new ETW macros.
 #define TRACE_EVENT_BEGIN_ETW(name, id, extra) \
     base::trace_event::TraceLog::AddTraceEventEtw( \
         TRACE_EVENT_PHASE_BEGIN, \
         name, reinterpret_cast<const void*>(id), extra)
 
 #define TRACE_EVENT_END_ETW(name, id, extra) \
     base::trace_event::TraceLog::AddTraceEventEtw( \
         TRACE_EVENT_PHASE_END, \
         name, reinterpret_cast<const void*>(id), extra)
 
 #define TRACE_EVENT_INSTANT_ETW(name, id, extra) \
     base::trace_event::TraceLog::AddTraceEventEtw( \
         TRACE_EVENT_PHASE_INSTANT, \
         name, reinterpret_cast<const void*>(id), extra)
 
 template <typename Type>
 struct DefaultSingletonTraits;
 
 namespace base {
 
 class WaitableEvent;
 class MessageLoop;
 
 namespace trace_event {
 
+class TraceConfig;
+
 // For any argument of type TRACE_VALUE_TYPE_CONVERTABLE the provided
 // class must implement this interface.
 class BASE_EXPORT ConvertableToTraceFormat
     : public RefCounted<ConvertableToTraceFormat> {
  public:
   // Append the class info to the provided |out| string. The appended
   // data must be a valid JSON object. Strings must be properly quoted, and
   // escaped. There is no processing applied to the content after it is
   // appended.
   virtual void AppendAsTraceFormat(std::string* out) const = 0;
@@ skipping 269 matching lines @@
   // Clears both included/excluded pattern lists. This would be equivalent to
   // creating a CategoryFilter with an empty string, through the constructor.
   // i.e: CategoryFilter().
   //
   // When using an empty filter, all categories are considered included as we
   // are not excluding anything.
   void Clear();
 
  private:
   FRIEND_TEST_ALL_PREFIXES(TraceEventTestFixture, CategoryFilter);
+  friend class TraceConfig;

[dsinclair, 2015/05/08 17:44:50]

Can we just merge this straight into TraceConfig without the indirection step?

[Zhen Wang, 2015/05/13 00:01:11]

Many places are using CategoryFilter. Do you mean just update all the
implementations with CategoryFilter by TraceConfig directly in this CL? I planed
to do them in 2 steps. Doing them in one step leads to a much longer CL.

 
   // Returns true if category is enable according to this filter.
   bool IsCategoryEnabled(const char* category_name) const;
 
   static bool IsEmptyOrContainsLeadingOrTrailingWhitespace(
       const std::string& str);
 
   void Initialize(const std::string& filter_string);
   void WriteString(const StringList& values,
                    std::string* out,
                    bool included) const;
   void WriteString(const StringList& delays, std::string* out) const;
   bool HasIncludedPatterns() const;
 
-  StringList included_;
-  StringList disabled_;
-  StringList excluded_;
-  StringList delays_;
+  scoped_ptr<TraceConfig> config_;

[nednguyen, 2015/05/11 17:59:28]

Why not just make this a value instead of scoped_ptr?

[Zhen Wang, 2015/05/13 00:01:11]

There will be extra copies in the constructors, right? Or that does not matter
much?

[nednguyen, 2015/05/13 18:16:31]

Chatted offline. It doesn't matter much & the compiler probably will do the
right thing for you.

[Zhen Wang, 2015/05/13 20:59:16]

Done.

 };
 
 class TraceSamplingThread;
 
 // Options determines how the trace buffer stores data.
 enum TraceRecordMode {
   // Record until the trace buffer is full.
   RECORD_UNTIL_FULL,
 
   // Record until the user ends the trace. The trace buffer is a fixed size
@@ skipping 42 matching lines @@
   // Returns true on success.
   bool SetFromString(const std::string& options_string);
 
   std::string ToString() const;
 
   TraceRecordMode record_mode;
   bool enable_sampling;
   bool enable_systrace;
 };
 
+class BASE_EXPORT TraceConfig {
+ public:
+  typedef std::vector<std::string> StringList;

[dsinclair, 2015/05/08 17:44:49]

Is this worth typedef'ing?

[Zhen Wang, 2015/05/13 00:01:11]

I was following the style used in CategoryFilter, so that it is consistent.

+
+  // The default trace config, used when none is provided.
+  // Allows all categories through, except if they end in the suffix 'Debug' or

[dsinclair, 2015/05/08 17:44:49]

Is this true? We don't want to allow disable-by-default- through by default ....

[Zhen Wang, 2015/05/13 00:01:11]

This is converted from kDefaultCategoryFilterString. See above in
CategoryFilter. Now kDefaultTraceConfigString is removed as we will use
InitializeDefault().

+  // 'Test'.
+  static const char kDefaultTraceConfigString[];

[dsinclair, 2015/05/08 17:44:49]

Does this need to be in the public header? Can it be moved to an anonymous
namespace inside the cc file?

[Zhen Wang, 2015/05/13 00:01:11]

This is removed now as we will use InitializeDefault().

+
+  TraceConfig();
+
+  // Create TraceConfig object from CategoryFilter and TraceOptions.
+  TraceConfig(const CategoryFilter& cf, const TraceOptions& options);
+
+  // Create TraceConfig object from category filter and trace options strings.
+  TraceConfig(const std::string& category_filter_string,
+              const std::string& trace_options_string);
+
+  // |config_string| is a JSON formated string, containing both category filters

[dsinclair, 2015/05/08 17:44:49]

Can you add a similar comment to the above here:

Create TraceConfig object from JSON encoded trace config string.

[Zhen Wang, 2015/05/13 00:01:11]

Done.

+  // and trace options.
+  TraceConfig(const std::string& config_string);

[dsinclair, 2015/05/08 17:44:50]

nit: explicit

[Zhen Wang, 2015/05/13 00:01:11]

Done.

+
+  TraceConfig(const TraceConfig& tc);

[dsinclair, 2015/05/08 17:44:50]

nit: explicit

[Zhen Wang, 2015/05/13 00:01:10]

Done.

+
+  ~TraceConfig();
+
+  TraceConfig& operator=(const TraceConfig& rhs);

[dsinclair, 2015/05/08 17:44:49]

Do we use copy and assign for this?

[Zhen Wang, 2015/05/13 00:01:11]

Not now. But it may be used later when updating the implementations with
CategoryFilter to TraceConfig.

[dsinclair, 2015/05/21 13:42:07]

In that case, let's add the DISALLOW for now and we can remove it later if we
determine we need it.

[Zhen Wang, 2015/05/22 23:38:04]

COPY and ASSIGN are needed to implement CategoryFilter for now. I can remove
them when cleaning all CategoryFilter implementations in the next CL.

[dsinclair, 2015/05/25 13:36:02]

I'm confused, above you said they aren't used? So, are they currently used or
not?

+
+  // Return a list of the synthetic delays specified in this category filter.
+  const StringList& GetSyntheticDelayValues() const;
+
+  // Convert TraceConfig to CategoryFilter.
+  void ToCategoryFilter(CategoryFilter& cf) const;

[dsinclair, 2015/05/08 17:44:49]

Are these ToXXX methods only for tests? I don't see them being called anywhere.
I'd prefer if we could get rid of them as it's making the public interfact of
this class a lot bigger.

Or, get rid of the bulk of them and just have toString

[Zhen Wang, 2015/05/13 00:01:11]

Removed now.

+
+  // Convert TraceConfig to TraceOptions.
+  void ToTraceOptions(TraceOptions& options) const;
+
+  // Convert TraceConfig to the dict representation of the TraceConfig.
+  void ToDict(base::DictionaryValue& dict) const;

[nednguyen, 2015/05/11 17:59:28]

Do we actually need this API?

[Zhen Wang, 2015/05/13 00:01:11]

I think this can be used by slow report.

[dsinclair, 2015/05/21 13:42:07]

Can we determine if it is or isn't? I'd prefer to leave it out of it isn't
needed.

[Zhen Wang, 2015/05/22 23:38:04]

Fadi has confirmed that this is not needed in the 1st stage of Deep Report.

Since it is used by ToString(). I moved it to private.

+
+  // Writes the string representation of the TraceConfig. The string is JSON
+  // formatted.
+  std::string ToString() const;
+
+  // Write the string representation of the CategoryFilter part.
+  std::string ToCategoryFilterString() const;
+
+  // Write the string representation of the TraceOptions part.
+  std::string ToTraceOptionsString() const;
+
+  // Returns true if at least one category in the list is enabled by this

[dsinclair, 2015/05/08 17:44:49]

How does this interact with disabled categories? If I do "foo,-bar" and
TRACE("foo,bar") will it be considered enabled?

[Zhen Wang, 2015/05/13 00:01:11]

Yes, it will be considered enabled. This comes from the original implementation
in CategoryFilter. See 
https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/t...

And I add one more check for this in TraceConfigTest.TraceConfigFromValidString:
EXPECT_TRUE(tc.IsCategoryGroupEnabled("included,excluded"));

+  // trace config.
+  bool IsCategoryGroupEnabled(const char* category_group) const;
+
+  // Merges config with the current TraceConfig
+  void Merge(const TraceConfig& config);

[dsinclair, 2015/05/08 17:44:49]

Will this go away when CategoryFilter is removed? Or will this have to live on?

[Zhen Wang, 2015/05/13 00:01:11]

This will live on. It is used by TraceLog. See
https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/t...

+
+  void Clear();

[dsinclair, 2015/05/08 17:44:50]

Is this needed? Or just for tests? If for tests, delete it and just create a new
object.

[Zhen Wang, 2015/05/13 00:01:11]

Clear() will be used by TraceLog. See
https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/t...

+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(TraceConfigTest, TraceConfigFromAndToString);
+  FRIEND_TEST_ALL_PREFIXES(TraceConfigTest, TraceConfig);
+  friend class CategoryFilter;
+
+  void Initialize(const std::string& config_string);
+  void SetCategoriesFromList(StringList& categories,
+                             const base::ListValue& list);
+  void AddCategoryToDict(base::DictionaryValue& dict,
+                         const char* param,
+                         const StringList& categories) const;
+
+  // Returns true if category is enable according to this trace config.
+  bool IsCategoryEnabled(const char* category_name) const;
+
+  static bool IsEmptyOrContainsLeadingOrTrailingWhitespace(
+      const std::string& str);
+
+  bool HasIncludedPatterns() const;
+
+  TraceRecordMode record_mode_;
+  bool enable_sampling_;
+  bool enable_systrace_;

[dsinclair, 2015/05/08 17:44:50]

bool enable_sampling_ : 1;
bool enable_systrace_ : 1;

[Zhen Wang, 2015/05/13 00:01:11]

Those two members will be used in TraceConfig::Initialize:

dict->GetBoolean(kEnableSamplingParam, &enable_sampling_)
dict->GetBoolean(kEnableSystraceParam, &enable_systrace_)

Specify them as bit field will make them not addressable.

[dsinclair, 2015/05/21 13:42:07]

Sure, but we can also easily change the initialize call to write into a temp
boolean and then assign the value.

We're using more space then needed here, although it isn't a lot, but why not
save the extra byte?

[Zhen Wang, 2015/05/22 23:38:04]

Done.

+
+  StringList included_categories_;
+  StringList disabled_categories_;
+  StringList excluded_categories_;
+  StringList synthetic_delays_;
+};

[dsinclair, 2015/05/08 17:44:50]

Should this have a DISALLOW_COPY_AND_ASSIGN?

[Zhen Wang, 2015/05/13 00:01:10]

Existing CategoryFilter has copy constructor and operator= function defined. So
TraceConfig will also need them.

[dsinclair, 2015/05/21 13:42:07]

If it isn't needed let, let's put in the DISALLOW. We can remove it in the
future if needed, but it's possible CategoryFilter just never had one.

[Zhen Wang, 2015/05/22 23:38:04]

COPY and ASSIGN are needed to implement CategoryFilter for now. I can remove
them when cleaning all CategoryFilter implementations in the next CL.

+
 struct BASE_EXPORT TraceLogStatus {
   TraceLogStatus();
   ~TraceLogStatus();
   size_t event_capacity;
   size_t event_count;
 };
 
 class BASE_EXPORT TraceLog {
  public:
   enum Mode {
@@ skipping 379 matching lines @@
   subtle::AtomicWord generation_;
   bool use_worker_thread_;
 
   DISALLOW_COPY_AND_ASSIGN(TraceLog);
 };
 
 }  // namespace trace_event
 }  // namespace base
 
 #endif  // BASE_TRACE_EVENT_TRACE_EVENT_IMPL_H_