tracing: simplify lifetime of TraceEventFilter(s)

base/trace_event/event_filter_registry.h

Index: base/trace_event/event_filter_registry.h
diff --git a/base/trace_event/event_filter_registry.h b/base/trace_event/event_filter_registry.h
new file mode 100644
index 0000000000000000000000000000000000000000..bbb2697223b1663f455da2b71a886dcb4f880cd5
--- /dev/null
+++ b/base/trace_event/event_filter_registry.h
@@ -0,0 +1,108 @@
+// Copyright 2016 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_EVENT_FILTER_REGISTRY_H_
+#define BASE_TRACE_EVENT_EVENT_FILTER_REGISTRY_H_
+
+#include <stddef.h>
+#include <stdint.h>
+
+#include <memory>
+
+#include "base/base_export.h"
+#include "base/logging.h"
+#include "base/macros.h"
+
+namespace base {
+namespace trace_event {
+
+class TraceEventFilter;
+
+// Keeps track of all filters and manages their lifetime in a circular FIFO.
+//
+// When a filter is registered via RegisterFilter():
+// the registry takes ownership and guarantees that: (1) the filter will live at
+// least until the unregister call; (2) the index of the filter, returned by
+// Iterator.GetIndex(), will be stable throughout all the lifetime of the
+// filter. (2) allows TraceCategory instances to keep a fast and cheap bitmap
+// cache of enabled filters.
+//
+// When UnregisterAllFilters() is called:
+// the registry stops immediately returning any filter from GetActiveFilters()
+// but does NOT destroy any filter. Instead it will delay the destruction of
+// each filter until other kMaxFilters filters are re-added to the registry.
+//
+// The rationale of this lifetime model is: at the time UnregisterAllFilters()
+// is called some TRACE_EVENT macros might still be using the filters on other
+// threads. We can't take locks or use atomics when invoking those filters as
+// it would impact performance of TRACE_EVENT macros. So we defer their
+// destruction until the very last moment.

[ssid, 2016/12/12 04:50:18]

When I proposed the model of leaking filters to Oystein, he was strongly against
leaking / keeping alive 32 filter objects that can contain list of strings and
now it also contains the config. This could use a bit of memory. That is the
reason we went for a model where filters are destroyed when next session starts.
Each EventNameFilter object has EventNamesWhitelist. We might be whitelisting
lot of events.
New filters can be added that are big objects.

[Primiano Tucci (use gerrit), 2016/12/12 14:38:54]

Even if each filter contains a whitelist of 100 strings of 10 chars each, it
would be ~1K per filter -> 32k for the 32 filters. 
If we really want to free this memory as soon as possible, but not too soon, I
think the best way is to:
- Add a method: DestroyUnregisteredFilters() here, which deletes all the !active
filters.
- Post a call to that method at some point after tracing has been disabled.


We have to just come up with a reasonable approach here. The race window is
quite tiny if you think about that: is between the read of the bitmap and the
actual dereferencign of the filter.
In theory we could PostDelayedTask(one second,  DestroyUnregisteredFilters)
after tracing has been disabled and that would sovle all the problems.
The only thing that that "one second" sucks. Is not the 1s itself, is any
arbitrary amount of time really.
But I cannot think to any other option (without start making the use of filters
locked, which is a no-go)

WDYT?

[ssid, 2016/12/12 21:25:53]

So, I think we could just destroy at SetEnabled. We have an assumption that we
do not have a race between SetEnabled and SetDisabled in lot of places like
memory-infra. the OnTracelogEnabled calls are made outside lock and if tracing
was enabled in that gap, then we would crash at MDM.
We also have a DCHECK(!flush_task_runner_); which means we will crash if enabled
just after disabling.

You should also note that we do a FlushCurrentThread call on all the threads on
the process after a SetDisabled call. The only case where the crash can happen
is that if a new thread which is not registered yet tried to access the filter.
So, the time difference here is not really small and it's safe.

But, in any case we will still be leaking some filters that were added in last
tracing session since we don't clear till next session. So, I am fine with the
existing code leaking filters. Just that we will be leaking more filters now
(from multiple sessions).

It was Oystein's opinion. Not sure what he thinks is better.

+class BASE_EXPORT EventFilterRegistry {
+ public:
+  // Usage: for (auto it = GetActiveFilters(); it.IsValid(); it.MoveNext()).
+  // Not worth the complexity of a full std::iterator.
+  class Iterator {
+   public:
+    bool IsValid() const;
+    void MoveNext();
+    uint32_t GetIndex() const;
+    const TraceEventFilter* operator->() const;
+    const TraceEventFilter& operator*() const { return *(operator->()); };
+
+   private:
+    friend class EventFilterRegistry;
+    Iterator(uint32_t start, uint32_t pos);
+
+    // The position of the cursor in the ring buffer at the time
+    // GetActiveFilters() was called.
+    const uint32_t start_;
+
+    // Offset in the buffer relative to start_. 0 <= offset < kMaxFilters.
+    uint32_t offset_;
+  };
+
+  // Returns a filter from its index (obtained through Iterator::GetIndex()).
+  static TraceEventFilter* Get(uint32_t index) {
+    DCHECK_LT(index, kMaxFilters);
+    DCHECK(filters_[index].filter);
+    return filters_[index].filter;
+  }
+
+  // Calls to the methods below must be serialized by the caller:
+  // either called on the same thread or holding a lock.
+
+  // Registers a filter and guarantees to keep it alive at least until the
+  // unregistration. It causes a crash via CHECK() if attempting to register
+  // more more than kMaxFilters.
+  static void RegisterFilter(std::unique_ptr<TraceEventFilter>);
+
+  // Unregisters all filters without destroying them.
+  static void UnregisterAllFilters();
+
+  // Returns a sequence of filters that have not been unregistered.
+  static Iterator GetActiveFilters();
+
+  static void ResetForTesting();
+
+ private:
+  struct FilterEntry {
+    // Owned pointer to the filter instance. When nullptr indicates a free slot.
+    // Can't be a unique_ptr as that would create static initializers.
+    TraceEventFilter* filter;
+
+    // When false it indicates that the filter has been unregister and hence the
+    // |filter| can be freed and the slot reused.
+    bool active;
+  };
+  static constexpr uint32_t kMaxFilters = 32;
+  static FilterEntry filters_[kMaxFilters];

[ssid, 2016/12/12 04:50:18]

Should you annotate this object as leaked, or the sanitizers will complain?
ANNOTATE_LEAKING_OBJECT_PTR

[Primiano Tucci (use gerrit), 2016/12/12 14:38:54]

I discovered recently that ASan doesn't complain about memory still reachable by
global objects.
In other words, for Asan this is NOT a leak:

char* g_var;

function() {
 g_var = malloc(...)
}

if you call function() only once.

However, if you call function() twice or more, it becomes a leak (and asan will
complain) becuase now you have allocated memory which is not reachable by
anything else.

In our case, all the newly allocated objects are referenced from filters_, so
there is no need for an annotation

[ssid, 2016/12/12 21:25:53]

Okay thanks!

+  static uint32_t filters_index_;
+
+  DISALLOW_COPY_AND_ASSIGN(EventFilterRegistry);
+};
+
+}  // namespace trace_event
+}  // namespace base
+
+#endif  // BASE_TRACE_EVENT_EVENT_FILTER_REGISTRY_H_