Issue 4125 - [emscripten] Convert filter classes to C++

compiled/Filter.cpp

 #include "Filter.h"
 #include "CommentFilter.h"
 #include "InvalidFilter.h"
 #include "RegExpFilter.h"
+#include "BlockingFilter.h"
 #include "WhitelistFilter.h"
 #include "ElemHideBase.h"
 #include "ElemHideFilter.h"
 #include "ElemHideException.h"
 #include "CSSPropertyFilter.h"
 #include "StringMap.h"
 
 namespace
 {
   StringMap<Filter*> knownFilters(8192);
@@ skipping 32 matching lines @@
     // Remove trailing spaces
     for (; end > 0; end--)
       if (text[end - 1] != ' ')
         break;
 
     // Set new string boundaries
     text.reset(text, start, end - start);
   }
 }
 
-Filter::Filter(const String& text)
-    : ref_counted(), mText(text)

[sergei, 2016/02/17 12:54:37]

`ref_counted()` is not necessary here.

[Wladimir Palant, 2016/02/18 16:06:41]

I preferred to spell this out explicitly nevertheless. Anyway, removed.

+Filter::Filter(Type type, const String& text)
+    : mType(type), mText(text)
 {
   annotate_address(this, "Filter");
 }
 
 Filter::~Filter()
 {
-  // TODO: This should be removing from knownFilters
+  knownFilters.erase(mText);
 }
 
 OwnedString Filter::Serialize() const
 {
   OwnedString result(u"[Filter]\ntext="_str);
   result.append(mText);
   result.append(u'\n');
-  return std::move(result);

[sergei, 2016/02/17 12:54:36]

std::move is not necessary here.
BTW, in C++, here will be copy elision.

[Wladimir Palant, 2016/02/18 16:06:42]

Done.

+  return result;
 }
 
 Filter* Filter::FromText(DependentString& text)
 {
   NormalizeWhitespace(text);

[sergei, 2016/02/17 12:54:33]

For me personally, the approach here is really confusing in terms of C++ because
it can easily lead to something unexpected.

Eventually we copy `text` to `Filter::mText`, so I would propose to pass `text`
as a constant reference, copy it to `OwnedString` (`Filter::mText`) (here we can
trim it) and then operate on the `OwnedString` to remove undesired characters in
the middle and don't modify `DependentString`. BTW, instead of passing it as a
constant reference, it can be even r-value `OwnedString&&`.
I guess it won't cause a significant overhead.

Having that, I guess, we will be able to move non-constant methods from `String`
to `OwnedString` class and remove `DependentString(String& str,...)` and
`DependentString(value_type* buf, size_type len)` (constructors which accept
non-constant buffers).

What do you think about it?

[Wladimir Palant, 2016/02/18 16:06:43]

I've been through multiple iterations here, and the current version is the one
that saves significant overhead. While messing with the incoming buffers isn't
the best coding style, we can do so "for free" - they are allocated on the stack
and will be automatically released anyway as soon as we return. Once we allocate
our own buffer however we better make everything right - reallocating will be
expensive.

For example, how do we trim leading whitespace with an owned buffer? We have the
choice between moving the content and keeping two pointers around (one for the
buffer we need to release, another one for start of the content).
DependentString allows doing this very efficiently.

[sergei, 2016/02/22 12:45:36]

Clear.

BTW, can we avoid copying of strings here at all? In the extension we read
filters from a file, and we can read the file into a contiguous buffer, then we
can create `DependentString`s which use that buffer and even modify it in place
(it's our buffer on the heap). It looks fragile but it should be very efficient.
Of course we should do something to have an ability to add and remove filters.
We can do it similar way as we do it now. Currently we find `start` and `end` of
the expected trimmed and normalized string in `NormalizeWhitespace`, then call
`DependentString::reset(text, start, end - start);` and then copy the piece of
buffer determined by fields of `DependentString` into `Filter::mText`. I
proposed to find offset (`start`) and the length (`start - end`) of the expected
trimmed string and then copy it into `Filter::mText` without modifying of
`DependentString` and its buffer, and then remove undesired characters in the
middle of the string. The latter action means the possibility for overhead in
the unused tail of `OwnedString`, how often does it actually happen?. And to do
that we need to add `OwnedString::OwnedString(const String& src, size_type
offset, size_type length)` or use `OwnedString(DependentString(text, offset,
length))`.

Anyway, since we know what is happening on the caller side, I would propose to
leave it as is because it's more descriptive now.

[Wladimir Palant, 2016/02/23 12:37:20]

Reading that file will eventually move into C++ so at that point we could try
tricks like that. There is an encoding issue however: the file uses UTF-8 which
we will have to decode anyway. And even without it, it would merely lead to
memory fragmentation - that file contains way more info that the filter strings,
and we wouldn't be able to reuse that memory easily. I think a fixed buffer
(e.g. 128 kB) which we would read chunks of the file into, parse and then reuse
will be way more memory-efficient.
I had this implemented like that, to some degree. Main problem is: we have to
perform the normalization before creating the filter, in order to do the lookup
in knownFilters. We only create the filter if that lookup yields no result. I'd
rather not create an owned buffer only to free it again because we didn't need
it.

[sergei, 2016/02/23 15:07:24]

Acknowledged.

   if (text.empty())
     return nullptr;
 
   // Parsing also normalizes the filter text, so it has to be done before the
   // lookup in knownFilters.
   union
   {
     RegExpFilterData regexp;
     ElemHideData elemhide;
   } data;
-  OwnedString error;
+  DependentString error;
 
   Filter::Type type = CommentFilter::Parse(text);
   if (type == Filter::Type::UNKNOWN)
     type = ElemHideBase::Parse(text, data.elemhide);
   if (type == Filter::Type::UNKNOWN)
     type = RegExpFilter::Parse(text, error, data.regexp);
 
-  FilterPtr filter(GetKnownFilter(text));
-  if (filter)
-    return filter;
+  auto knownFilter = knownFilters.find(text);
+  if (knownFilter)
+  {
+    knownFilter->second->AddRef();
+    return knownFilter->second;
+  }
 
+  FilterPtr filter;
   switch (type)
   {
     case Filter::Type::COMMENT:
       filter = new CommentFilter(text);
       break;
     case Filter::Type::INVALID:
       filter = new InvalidFilter(text, error);
       break;
     case Filter::Type::BLOCKING:
-      filter = new RegExpFilter(text, data.regexp);
+      filter = new BlockingFilter(text, data.regexp);
       break;
     case Filter::Type::WHITELIST:
       filter = new WhitelistFilter(text, data.regexp);
       break;
     case Filter::Type::ELEMHIDE:
       filter = new ElemHideFilter(text, data.elemhide);
       break;
     case Filter::Type::ELEMHIDEEXCEPTION:
       filter = new ElemHideException(text, data.elemhide);
       break;
     case Filter::Type::CSSPROPERTY:
       filter = new CSSPropertyFilter(text, data.elemhide);
-      if (reinterpret_cast<CSSPropertyFilter*>(filter.get())->IsGeneric())

[sergei, 2016/02/17 12:54:34]

it's better to use `static_cast` here.

[Wladimir Palant, 2016/02/18 16:06:44]

Done.

-        filter = new InvalidFilter(text,
-            u"No active domain specified for CSS property filter"_str);
+      if (static_cast<CSSPropertyFilter*>(filter.get())->IsGeneric())
+        filter = new InvalidFilter(text, u"filter_cssproperty_nodomain"_str);
       break;
     default:
       // This should never happen but just in case
       return nullptr;
   }
 
+  // This is a hack: we looked up the entry using text but create it using
+  // filter->mText. This works because both are equal at this point. However,
+  // text refers to a temporary buffer which will go away.
   enter_context("Adding to known filters");
-  knownFilters[filter->mText] = filter.get();
+  knownFilter.assign(filter->mText, filter.get());
   exit_context();
 
-  // TODO: We intentionally leak the filter here - currently it won't be used
-  // for anything and would be deleted immediately.
-  filter->AddRef();
-
-  return filter;
+  return filter.release();
 }
-
-Filter* Filter::GetKnownFilter(const String& text)
-{
-  auto it = knownFilters.find(text);
-  if (it != knownFilters.end())
-    return it->second;
-  else
-    return nullptr;
-}