Optimize phishing page term feature extraction.

Optimize phishing page term feature extraction.

We've been seeing in the histograms that for some users, page feature
extraction is taking much longer than we would like, up to 200ms between
stops. This could possibly contribute to jankiness in the UI. These are some
timings from my desktop for how long it takes to extract features before and
after these changes. Obviously this is on much better hardware than the use case
we are worried about, but hopefully the increase is speed is proportional.

           Before  After
CNN        19       7.5    
ESPN       22       9.5
Google     12       4
Salon      40       12

BUG=
TEST=Ran associated unit tests.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96097

[Garrett Casto, 2011-08-02 02:30:03]

Brett, I'm happy to move the base/* changes to another CL if you'd like. I
thought that I would keep them together for now to give context for the change.

[Paweł Hajdan Jr., 2011-08-02 17:10:33]

Drive-by with testing comments.

http://codereview.chromium.org/7549003/diff/1/base/wide_string_piece_unittest.cc
File base/wide_string_piece_unittest.cc (right):

http://codereview.chromium.org/7549003/diff/1/base/wide_string_piece_unittest...
base/wide_string_piece_unittest.cc:15: #define CMP_Y(op, x, y)                  
                            \
Why do we need those? Gtest makes it possible to do custom comparisons without
additional macros. Please read the friendly manual. Hint:
http://code.google.com/p/googletest/wiki/V1_6_AdvancedGuide#Predicate_Asserti...

[Garrett Casto, 2011-08-02 21:52:28]

http://codereview.chromium.org/7549003/diff/1/base/wide_string_piece_unittest.cc
File base/wide_string_piece_unittest.cc (right):

http://codereview.chromium.org/7549003/diff/1/base/wide_string_piece_unittest...
base/wide_string_piece_unittest.cc:15: #define CMP_Y(op, x, y)                  
                            \
On 2011/08/02 17:10:33, Paweł Hajdan Jr. wrote:
> Why do we need those? Gtest makes it possible to do custom comparisons without
> additional macros. Please read the friendly manual. Hint:
>
http://code.google.com/p/googletest/wiki/V1_6_AdvancedGuide#Predicate_Asserti...

This was because I copied the meat of this test from string_piece_unittest.cc.
Since I didn't implemented a compare function, using these macros is not really
necessary, so I've removed them.

[Paweł Hajdan Jr., 2011-08-02 22:03:18]

Code I commented in the drive-by LGTM with a comment.

http://codereview.chromium.org/7549003/diff/3001/base/wide_string_piece_unitt...
File base/wide_string_piece_unittest.cc (right):

http://codereview.chromium.org/7549003/diff/3001/base/wide_string_piece_unitt...
base/wide_string_piece_unittest.cc:15:
ASSERT_TRUE(WideStringPiece(ASCIIToUTF16("")) ==
How about ASSERT_EQ? Up to you.

[Garrett Casto, 2011-08-02 22:03:59]

I just realized that this doesn't work on Windows as is. Might want to hold of
on reviews for a bit.

[Garrett Casto, 2011-08-03 17:33:27]

On 2011/08/02 22:03:59, Garrett Casto wrote:
> I just realized that this doesn't work on Windows as is. Might want to hold of
> on reviews for a bit.

Alright, sorry for the delay. Please take a look.

[noelutz, 2011-08-03 19:48:55]

LGTM

Nicely done!
noe.

On 2011/08/03 17:33:27, Garrett Casto wrote:
> On 2011/08/02 22:03:59, Garrett Casto wrote:
> > I just realized that this doesn't work on Windows as is. Might want to hold
of
> > on reviews for a bit.
> 
> Alright, sorry for the delay. Please take a look.

[brettw, 2011-08-03 20:47:38]

http://codereview.chromium.org/7549003/diff/6001/base/memory/mru_cache.h
File base/memory/mru_cache.h (right):

http://codereview.chromium.org/7549003/diff/6001/base/memory/mru_cache.h#newc...
base/memory/mru_cache.h:36: struct StandardMap {
How about calling this MRUCacheStandardMap since putting "StandardMap" in the
base namespace seems awfully generic.

http://codereview.chromium.org/7549003/diff/6001/base/wide_string_piece.h
File base/wide_string_piece.h (right):

http://codereview.chromium.org/7549003/diff/6001/base/wide_string_piece.h#new...
base/wide_string_piece.h:22: class BASE_API WideStringPiece {
This should be called StringPiece16. "Wide" is what we use for wchar_t, which is
different on Mac & Linux.

I would actually just put this in string_piece.h header and not bother with a
separate one for the new class.

[Garrett Casto, 2011-08-04 00:03:51]

Take another look

http://codereview.chromium.org/7549003/diff/6001/base/memory/mru_cache.h
File base/memory/mru_cache.h (right):

http://codereview.chromium.org/7549003/diff/6001/base/memory/mru_cache.h#newc...
base/memory/mru_cache.h:36: struct StandardMap {
On 2011/08/03 20:47:38, brettw wrote:
> How about calling this MRUCacheStandardMap since putting "StandardMap" in the
> base namespace seems awfully generic.

Done. Changed the naming of the HashMap to MRUCacheHashMap as well.

http://codereview.chromium.org/7549003/diff/6001/base/wide_string_piece.h
File base/wide_string_piece.h (right):

http://codereview.chromium.org/7549003/diff/6001/base/wide_string_piece.h#new...
base/wide_string_piece.h:22: class BASE_API WideStringPiece {
On 2011/08/03 20:47:38, brettw wrote:
> This should be called StringPiece16. "Wide" is what we use for wchar_t, which
is
> different on Mac & Linux.
> 
> I would actually just put this in string_piece.h header and not bother with a
> separate one for the new class.

Done.

[brettw, 2011-08-05 23:00:23]

LGTM, I didn't really look at the phishing code changes.

http://codereview.chromium.org/7549003/diff/7004/base/string_piece.h
File base/string_piece.h (right):

http://codereview.chromium.org/7549003/diff/7004/base/string_piece.h#newcode318
base/string_piece.h:318: #define HASH_WIDE_STRING_PIECE(wsp) \
It doesn't look like regular StringPiece has a hash function, and with a trivial
addition of the "type" here you can use this same macro definition:
#define HASH_STRING_PIECE(StringPieceType, wsp)
 ...for(StringPieceType::const_iterator...
to define hashes below for StringPiece. Can you do that?

http://codereview.chromium.org/7549003/diff/7004/base/string_piece.h#newcode320
base/string_piece.h:320: for (base::StringPiece16::const_iterator i =
wsp.begin();           \
Fix whitespace.

[Garrett Casto, 2011-08-08 18:48:05]

http://codereview.chromium.org/7549003/diff/7004/base/string_piece.h
File base/string_piece.h (right):

http://codereview.chromium.org/7549003/diff/7004/base/string_piece.h#newcode318
base/string_piece.h:318: #define HASH_WIDE_STRING_PIECE(wsp) \
On 2011/08/05 23:00:23, brettw wrote:
> It doesn't look like regular StringPiece has a hash function, and with a
trivial
> addition of the "type" here you can use this same macro definition:
> #define HASH_STRING_PIECE(StringPieceType, wsp)
>  ...for(StringPieceType::const_iterator...
> to define hashes below for StringPiece. Can you do that?

Done.

http://codereview.chromium.org/7549003/diff/7004/base/string_piece.h#newcode320
base/string_piece.h:320: for (base::StringPiece16::const_iterator i =
wsp.begin();           \
On 2011/08/05 23:00:23, brettw wrote:
> Fix whitespace.

Done.

[Brian Ryner, 2011-08-08 22:08:00]

Very nice overall, just some minor comments.

http://codereview.chromium.org/7549003/diff/18002/base/memory/mru_cache.h
File base/memory/mru_cache.h (right):

http://codereview.chromium.org/7549003/diff/18002/base/memory/mru_cache.h#new...
base/memory/mru_cache.h:26: #include "base/hash_tables.h"
Keep these in alphabetical order?

http://codereview.chromium.org/7549003/diff/18002/base/memory/mru_cache.h#new...
base/memory/mru_cache.h:280: // the map type instead of std::map. Note that you
your KeyType must be hashable
extra "you"

http://codereview.chromium.org/7549003/diff/18002/base/string_piece.h
File base/string_piece.h (right):

http://codereview.chromium.org/7549003/diff/18002/base/string_piece.h#newcode222
base/string_piece.h:222: void set(const void* data, size_type len) {
As we discussed offline, unless this particular version is needed for something,
we should probably remove it.  It allows implicit casting from char*, which
would be highly confusing.

http://codereview.chromium.org/7549003/diff/18002/base/string_piece.h#newcode230
base/string_piece.h:230: // StringPiece claims that this is bad when data() is
NULL, but unittesting
So the CheckNULL test verifies that this is true on all of the platforms we
support?

http://codereview.chromium.org/7549003/diff/18002/chrome/renderer/safe_browsi...
File chrome/renderer/safe_browsing/phishing_term_feature_extractor.cc (right):

http://codereview.chromium.org/7549003/diff/18002/chrome/renderer/safe_browsi...
chrome/renderer/safe_browsing/phishing_term_feature_extractor.cc:96:
negative_word_cache_(1000 /* max_size */),
Maybe make this a class constant, like the other knobs that we have for this
extractor?

http://codereview.chromium.org/7549003/diff/18002/chrome/renderer/safe_browsi...
chrome/renderer/safe_browsing/phishing_term_feature_extractor.cc:202:
negative_word_cache_.Clear();
Hm... I could imagine there being some benefit to maintaining the cache across
pages, but perhaps it's not as big as the win from using StringPiece and
avoiding the string copies on lookup.  We could have it both ways, I guess, if
we kept separate internal storage for the strings that are in
negative_word_cache_, but maybe not worth it.

[Garrett Casto, 2011-08-08 23:19:51]

http://codereview.chromium.org/7549003/diff/18002/base/memory/mru_cache.h
File base/memory/mru_cache.h (right):

http://codereview.chromium.org/7549003/diff/18002/base/memory/mru_cache.h#new...
base/memory/mru_cache.h:26: #include "base/hash_tables.h"
On 2011/08/08 22:08:00, Brian Ryner wrote:
> Keep these in alphabetical order?

Done.

http://codereview.chromium.org/7549003/diff/18002/base/memory/mru_cache.h#new...
base/memory/mru_cache.h:280: // the map type instead of std::map. Note that you
your KeyType must be hashable
On 2011/08/08 22:08:00, Brian Ryner wrote:
> extra "you"

Done.

http://codereview.chromium.org/7549003/diff/18002/base/string_piece.h
File base/string_piece.h (right):

http://codereview.chromium.org/7549003/diff/18002/base/string_piece.h#newcode222
base/string_piece.h:222: void set(const void* data, size_type len) {
On 2011/08/08 22:08:00, Brian Ryner wrote:
> As we discussed offline, unless this particular version is needed for
something,
> we should probably remove it.  It allows implicit casting from char*, which
> would be highly confusing.

Done.

http://codereview.chromium.org/7549003/diff/18002/base/string_piece.h#newcode230
base/string_piece.h:230: // StringPiece claims that this is bad when data() is
NULL, but unittesting
On 2011/08/08 22:08:00, Brian Ryner wrote:
> So the CheckNULL test verifies that this is true on all of the platforms we
> support?

Yeah, that's the idea. It doesn't crash in the tests that I've seen.

http://codereview.chromium.org/7549003/diff/18002/chrome/renderer/safe_browsi...
File chrome/renderer/safe_browsing/phishing_term_feature_extractor.cc (right):

http://codereview.chromium.org/7549003/diff/18002/chrome/renderer/safe_browsi...
chrome/renderer/safe_browsing/phishing_term_feature_extractor.cc:96:
negative_word_cache_(1000 /* max_size */),
On 2011/08/08 22:08:00, Brian Ryner wrote:
> Maybe make this a class constant, like the other knobs that we have for this
> extractor?

Done.

http://codereview.chromium.org/7549003/diff/18002/chrome/renderer/safe_browsi...
chrome/renderer/safe_browsing/phishing_term_feature_extractor.cc:202:
negative_word_cache_.Clear();
On 2011/08/08 22:08:00, Brian Ryner wrote:
> Hm... I could imagine there being some benefit to maintaining the cache across
> pages, but perhaps it's not as big as the win from using StringPiece and
> avoiding the string copies on lookup.  We could have it both ways, I guess, if
> we kept separate internal storage for the strings that are in
> negative_word_cache_, but maybe not worth it.

So I thought about this as well. It's rather hard to test because we don't know
what exactly people's browsing history is going to be like. I did do a bunch of
iterations in row at one point before I switched to using StringPiece16, and the
difference between the first task and subsequent ones was rather small, and
that's basically the optimal situation. Admittedly I didn't write down any
numbers, but I think that using StringPiece16 is better. If this comes up again
I can look into it.

[Brian Ryner, 2011-08-09 00:13:21]

LGTM

[commit-bot: I haz the power, 2011-08-09 23:30:47]

Change committed as 96097