Update visit analysis code:

Update visit analysis code:
-- limit visit history considered to 100k entries to avoid out of memory crashes
-- exclude backforward/homepage/intermediate pages in redirect chain
-- reduce minimum spacing of pages considered from 1s to 500ms
-- collect histogram of core transition type of qualifying page views
R=brettw, dominich
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=124779

[tburkard, 2012-03-02 21:07:29]

Brett: this should be very straightforward for you: you can see I am only
editing the existing class that I temporarily, and that I am limiting the
maximum # of history entries to 100k (whereas it used to be unlimited,
causing the crashes due to out of memory).

Dominic can review all the changed logic of stats collected based on our
discussion.

Thanks.

Timo

On Fri, Mar 2, 2012 at 1:05 PM, <tburkard@chromium.org> wrote:

> Reviewers: brettw, dominich,
>
> Description:
> Update visit analysis code:
> -- limit visit history considered to 100k entries to avoid out of memory
> crashes
> -- exclude backforward/homepage/**intermediate pages in redirect chain
> -- reduce minimum spacing of pages considered from 1s to 500ms
> -- collect histogram of core transition type of qualifying page views
> R=brettw, dominich
>
> Please review this at
https://chromiumcodereview.**appspot.com/9572021/<https://chromiumcodereview....
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src/<http://svn.chromium.org/chrome/trunk/src/>
>
> Affected files:
>  M     chrome/browser/history/visit_**database.cc
>
>
> Index: chrome/browser/history/visit_**database.cc
> ==============================**==============================**=======
> --- chrome/browser/history/visit_**database.cc    (revision 124710)
> +++ chrome/browser/history/visit_**database.cc    (working copy)
> @@ -75,15 +75,33 @@
>
>  private:
>   enum VA_EVENTS {
> -    VA_VISIT = 0,
> -    VA_PRERENDER_STARTED_1 = 1,
> -    VA_PRERENDER_USED_1 = 2,
> -    VA_PRERENDER_STARTED_3 = 3,
> -    VA_PRERENDER_USED_3 = 4,
> -    VA_PRERENDER_STARTED_5 = 5,
> -    VA_PRERENDER_USED_5 = 6,
> -    VA_EVENT_MAX = 7
> +    VA_VISIT = 10,
> +    VA_VISIT_EXCLUDE_BACK_FORWARD = 11,
> +    VA_VISIT_EXCLUDE_HOME_PAGE = 12,
> +    VA_VISIT_EXCLUDE_REDIRECT_**CHAIN = 13,
> +    VA_PRERENDER_STARTED_1 = 14,
> +    VA_PRERENDER_USED_1 = 15,
> +    VA_PRERENDER_STARTED_3 = 16,
> +    VA_PRERENDER_USED_3 = 17,
> +    VA_PRERENDER_STARTED_5 = 18,
> +    VA_PRERENDER_USED_5 = 19,
> +    VA_EVENT_MAX = 20
>   };
> +  bool IsBackForward(content::**PageTransition transition) {
> +    return transition & content::PAGE_TRANSITION_**FORWARD_BACK;
> +  }
> +  bool IsHomePage(content::**PageTransition transition) {
> +    return transition & content::PAGE_TRANSITION_HOME_**PAGE;
> +  }
> +  bool IsIntermediateRedirect(**content::PageTransition transition) {
> +    return (transition & content::PAGE_TRANSITION_IS_**REDIRECT_MASK) &&
> +        !(transition & content::PAGE_TRANSITION_**CHAIN_END);
> +  }
> +  bool ShouldExcludeTransitionForPred**iction(
> +      content::PageTransition transition) {
> +    return IsBackForward(transition) || IsHomePage(transition) ||
> +        IsIntermediateRedirect(**transition);
> +  }
>   void RecordEvent(VA_EVENTS event, base::Time timestamp) {
>     int64 ts = timestamp.ToInternalValue();
>     if (time_event_bitmaps_.count(ts) < 1)
> @@ -183,7 +201,23 @@
>   };
>   void ProcessVisit(const VisitRow& visit) {
>     VLOG(1) << "processing visit to " << visit.url_id;
> +    if (IsBackForward(visit.**transition)) {
> +      RecordEvent(VA_VISIT_EXCLUDE_**BACK_FORWARD, visit.visit_time);
> +      return;
> +    }
> +    if (IsHomePage(visit.transition)) {
> +      RecordEvent(VA_VISIT_EXCLUDE_**HOME_PAGE, visit.visit_time);
> +      return;
> +    }
> +    if (IsIntermediateRedirect(visit.**transition)) {
> +      RecordEvent(VA_VISIT_EXCLUDE_**REDIRECT_CHAIN, visit.visit_time);
> +      return;
> +    }
>     RecordEvent(VA_VISIT, visit.visit_time);
> +    UMA_HISTOGRAM_ENUMERATION(
> +        "Prerender.**LocalVisitCoreTransition",
> +        visit.transition & content::PAGE_TRANSITION_CORE_**MASK,
> +        content::PAGE_TRANSITION_LAST_**CORE + 1);
>     for (int i = 0; i < static_cast<int>(emulated_**
> prerender_managers_.size());
>          i++) {
>       emulated_prerender_managers_[**i]->MaybeRecordPrerenderUsed(**
> visit);
> @@ -199,6 +233,8 @@
>     URLID best_prerender = 0;
>     int best_count = 0;
>     for (int i = 0; i < static_cast<int>(visits_.size(**)); i++) {
> +      if (**ShouldExcludeTransitionForPred**
> iction(visits_[i].transition))
> +        continue;
>       VLOG(1) << visits_[i].visit_time.**ToInternalValue()/1000000 << " "
>               << visits_[i].url_id;
>       if (visits_[i].url_id == visit.url_id) {
> @@ -263,8 +299,8 @@
>   }
>   void FetchHistory() {
>     VLOG(1) << "VDB: fetching history";
> -    visit_database_->**GetAllVisitsInRange(base::**Time(), base::Time(),
> 0,
> -                                         &visits_);
> +    visit_database_->**GetAllVisitsInRange(base::**Time(), base::Time(),
> +                                         kMaxVisitRecords, &visits_);
>     VLOG(1) << "READ visits " << visits_.size();
>     if (logging::GetVlogVerbosity() >= 1) {
>       for (int i = 0; i < static_cast<int>(visits_.size(**)); i++) {
> @@ -277,9 +313,10 @@
>     visits_fetched_ = true;
>     MaybeProcessVisits();
>   }
> +  static const int kMaxVisitRecords = 100 * 1000;
>   static const int kFetchDelayMs = 30 * 1000;
>   static const int kPrerenderExpirationSeconds = 300;
> -  static const int kMinSpacingMilliseconds = 1000;
> +  static const int kMinSpacingMilliseconds = 500;
>   VisitDatabase* visit_database_;
>   base::OneShotTimer<**VisitAnalysis> timer_;
>   VisitVector visits_;
>
>
>

[dominich, 2012-03-02 21:30:55]

lgtm

[dominich, 2012-03-02 22:09:01]

Still LG, modulo the questions below not requiring changes.

https://chromiumcodereview.appspot.com/9572021/diff/1006/chrome/browser/histo...
File chrome/browser/history/visit_database.cc (right):

https://chromiumcodereview.appspot.com/9572021/diff/1006/chrome/browser/histo...
chrome/browser/history/visit_database.cc:90: bool
IsBackForward(content::PageTransition transition) {
if this becomes production code, these should all be const methods.

https://chromiumcodereview.appspot.com/9572021/diff/1006/chrome/browser/histo...
chrome/browser/history/visit_database.cc:97: return (transition &
content::PAGE_TRANSITION_CHAIN_END) == 0;
do navigations that are not part of a redirect chain have this flag set? or
should you be checking for the existence of a redirect chain and that this is
the end transition?

https://chromiumcodereview.appspot.com/9572021/diff/1006/chrome/browser/histo...
chrome/browser/history/visit_database.cc:317: static const int kMaxVisitRecords
= 100 * 1000;
is there a chance that this will still cause OOM failures in some cases?

[brettw, 2012-03-02 23:34:02]

lgtm