Limit the time spent on a single iteration of PhishingDOMFeatureExtractor.

Limit the time spent on a single iteration of PhishingDOMFeatureExtractor.

With this change, the feature extractor will keep track of how long it has been
working, and if it exceeds a predefined time limit, it will pause and post a
task to continue extraction.  This will allow other MessageLoop tasks to run on
the render thread.  There is also a total time limit, after which the extractor
will give up altogether -- this is a sanity check in case of a bug or an
especially large page.  Also includes some UMA histograms so that we can see
how well this performs in practice.

TEST=PhishingDOMFeatureExtractorTest.Continuation
BUG=none

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

[lzheng, 2010-08-20 20:54:53]

http://codereview.chromium.org/3130039/diff/1/4
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc (right):

http://codereview.chromium.org/3130039/diff/1/4#newcode168
chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc:168: // work. 
Log how long this takes, so we can tell if it's too slow.
Can you explain more about what time do you want to measure? It seems to me that
clock_->Now() - current_chunk_start_time is always the time from 144 to here,
which is super short.
Do you mean clock_->Now() - cur_frame_data_->start_time?

http://codereview.chromium.org/3130039/diff/1/6
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc
(right):

http://codereview.chromium.org/3130039/diff/1/6#newcode452
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc:452:
// Advance the clock 30 ms every 10 elements processed, 10 ms between chunks.
Explain that 10 elements is decided by
PhishingDOMFeatureExtractor::kClockCheckGranularity.
Instead of hard coding 10 elements and 50ms hard coded, I wonder if you could
use PhishingDOMFeatureExtractor::kClockCheckGranularity,
PhishingDOMFeatureExtractor::kMaxTimePerChunkMs and
PhishingDOMFeatureExtractor::kMaxTotalTimeMs in the test. Otherwise, when you
adjust those numbers, you'll have to magically tune the numbers below.

[Brian Ryner, 2010-08-20 21:14:17]

Please take another look.

http://codereview.chromium.org/3130039/diff/1/4
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc (right):

http://codereview.chromium.org/3130039/diff/1/4#newcode168
chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc:168: // work. 
Log how long this takes, so we can tell if it's too slow.
On 2010/08/20 20:54:53, lzheng wrote:
> Can you explain more about what time do you want to measure? It seems to me
that
> clock_->Now() - current_chunk_start_time is always the time from 144 to here,
> which is super short.
> Do you mean clock_->Now() - cur_frame_data_->start_time?

What I'm really trying to do is measure the time spent in the call to nextItem()
on line 165.  I made this more explicit in the comment.

http://codereview.chromium.org/3130039/diff/1/6
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc
(right):

http://codereview.chromium.org/3130039/diff/1/6#newcode452
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc:452:
// Advance the clock 30 ms every 10 elements processed, 10 ms between chunks.
On 2010/08/20 20:54:53, lzheng wrote:
> Explain that 10 elements is decided by
> PhishingDOMFeatureExtractor::kClockCheckGranularity.
> Instead of hard coding 10 elements and 50ms hard coded, I wonder if you could
> use PhishingDOMFeatureExtractor::kClockCheckGranularity,
> PhishingDOMFeatureExtractor::kMaxTimePerChunkMs and
> PhishingDOMFeatureExtractor::kMaxTotalTimeMs in the test. Otherwise, when you
> adjust those numbers, you'll have to magically tune the numbers below.

Added a comment.  I did play around with computing the clock values
programmatically from the constants, my feeling was that it made the test harder
to follow than using concrete values.  I'm ok with needing to update the test if
we change the constants.

[lzheng, 2010-08-20 21:22:08]

LGTM.

[marria, 2010-08-24 20:33:29]

LGTM

I like the UMA stats.  At first I was wondering if we would want them separated
by failed extractions and successful extractions, but I think the way you have
it now is fine.