[tracing] Introduce "allowed_dump_modes" for memory dump config

base/trace_event/memory_dump_manager.h

 // Copyright 2015 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_MEMORY_DUMP_MANAGER_H_
 #define BASE_TRACE_EVENT_MEMORY_DUMP_MANAGER_H_
 
 #include <stdint.h>
 
 #include <map>
@@ skipping 97 matching lines @@
                          const MemoryDumpCallback& callback);
 
   // Same as above (still asynchronous), but without callback.
   void RequestGlobalDump(MemoryDumpType dump_type,
                          MemoryDumpLevelOfDetail level_of_detail);
 
   // TraceLog::EnabledStateObserver implementation.
   void OnTraceLogEnabled() override;
   void OnTraceLogDisabled() override;
 
+  // Returns true if the dump mode is allowed for current tracing session.
+  bool IsDumpModeAllowed(MemoryDumpLevelOfDetail dump_mode) {
+    return allowed_dump_modes_.find(dump_mode) != allowed_dump_modes_.end();

[Primiano Tucci (use gerrit), 2016/06/08 15:48:30]

- Let's move this to the .cc file, there is no need to inline this.
- Can we make this such as: if the set if empty, then everything is allowed,
otherwise only what's in the set?
- I think .count(dump_mode) != 0 is a bit more legible and probably even more
performant, as it doesn't have to return an iterator.

[ssid, 2016/06/08 18:04:17]

Fixed.
This is already done in trace_config file where this kind of default behavior is
set for periodic dumps. Please see comment in trace_config.cc#545.
Done.

+  }
+
   // Returns the MemoryDumpSessionState object, which is shared by all the
   // ProcessMemoryDump and MemoryAllocatorDump instances through all the tracing
   // session lifetime.
   const scoped_refptr<MemoryDumpSessionState>& session_state() const {
     return session_state_;
   }
 
   // Returns a unique id for identifying the processes. The id can be
   // retrieved by child processes only when tracing is enabled. This is
   // intended to express cross-process sharing of memory dumps on the
@@ skipping 140 matching lines @@
     const scoped_refptr<SingleThreadTaskRunner> dump_thread_task_runner;
 
    private:
     DISALLOW_COPY_AND_ASSIGN(ProcessMemoryDumpAsyncState);
   };
 
   // Sets up periodic memory dump timers to start global dump requests based on
   // the dump triggers from trace config.
   class BASE_EXPORT PeriodicGlobalDumpTimer {
    public:
-    PeriodicGlobalDumpTimer();
+    explicit PeriodicGlobalDumpTimer(MemoryDumpManager* mdm);

[Primiano Tucci (use gerrit), 2016/06/08 15:48:30]

uh why cannot we rely on MDM being a singleton?

[ssid, 2016/06/08 18:04:17]

I did not want to do MDM::GetInstance() multiple times at each timer call also
makes code longer . If you prefer keeping Get calls here, i will remove this
change.

[Primiano Tucci (use gerrit), 2016/06/09 18:46:39]

GetInstance is pretty fast and generally nothing compared to the cost of all the
other things we do to create our dumps. I think it just makes it the code easier
to read.

     ~PeriodicGlobalDumpTimer();
 
     void Start(const std::vector<TraceConfig::MemoryDumpConfig::Trigger>&
                    triggers_list);
     void Stop();
 
     bool IsRunning();
 
    private:
     // Periodically called by the timer.
     void RequestPeriodicGlobalDump();
 
+    MemoryDumpManager* mdm_;
+
     RepeatingTimer timer_;
     uint32_t periodic_dumps_count_;
     uint32_t light_dump_rate_;
     uint32_t heavy_dump_rate_;
 
     DISALLOW_COPY_AND_ASSIGN(PeriodicGlobalDumpTimer);
   };
 
   static const int kMaxConsecutiveFailuresCount;
   static const char* const kSystemAllocatorPoolName;
@@ skipping 42 matching lines @@
   MemoryDumpProviderInfo::OrderedSet dump_providers_;
 
   // Shared among all the PMDs to keep state scoped to the tracing session.
   scoped_refptr<MemoryDumpSessionState> session_state_;
 
   MemoryDumpManagerDelegate* delegate_;  // Not owned.
 
   // When true, this instance is in charge of coordinating periodic dumps.
   bool is_coordinator_;
 
+  // Set of dump modes allowed for the current tracing session by the trace
+  // config.
+  std::set<MemoryDumpLevelOfDetail> allowed_dump_modes_;

[Primiano Tucci (use gerrit), 2016/06/08 15:48:30]

Can me put ths into the SessionState. Every time in the past we tried to add
state to MDM we ended up with some bug on corner cases when stopping/starting
trace. With SessionState we know that everything is inside there, so either all
the session state logic is buggy or not.
(And when I say "we ended up in bug" I mean mostly code that I wrote myself :) )

Actually wait, it's already there: the sessionstate contains a copy of the
traceconfig. The traceconfig contains the allowed dump modes.

[ssid, 2016/06/08 18:04:17]

Sorry, fixed this! I should be reading session state here.

+
   // Protects from concurrent accesses to the |dump_providers_*| and |delegate_|
   // to guard against disabling logging while dumping on another thread.
   Lock lock_;
 
   // Optimization to avoid attempting any memory dump (i.e. to not walk an empty
   // dump_providers_enabled_ list) when tracing is not enabled.
   subtle::AtomicWord memory_tracing_enabled_;
 
   // For time-triggered periodic dumps.
   PeriodicGlobalDumpTimer periodic_dump_timer_;
@@ skipping 39 matching lines @@
   }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MemoryDumpManagerDelegate);
 };
 
 }  // namespace trace_event
 }  // namespace base
 
 #endif  // BASE_TRACE_EVENT_MEMORY_DUMP_MANAGER_H_