Load the table of visited links from database file asynchronously.

Load the table of visited links from database file asynchronously.

Remove access to the database file from the UI thread and reduce
startup browser delays.

R=sky@chromium.org
BUG=24163
BUG=45591
Committed: https://crrev.com/af37203df5ddaf2b1491748ca4c0428a415e0f88
Cr-Commit-Position: refs/heads/master@{#365250}

[sky, 2015-10-21 21:32:39]

This means visits might not show correctly initially, right?
Have you measured the impacts on startup by doing this?

[sath, 2015-10-23 11:33:14]

On 2015/10/21 21:32:39, sky wrote:
> This means visits might not show correctly initially, right?
Yes right. We create the temporary table and pretend that we build the table
from history but actually in that moment we are loading data from the database
file asynchronously. However we don't lose any added or deleted urls. During the
load we collect them in temporary sets and modify the table accordingly once the
load is completed. 
> Have you measured the impacts on startup by doing this? 
We have collected trace of yandex browser startup and discovered that in the
some cases we lose about 200ms while loading file. Maybe it was caused by some
kind of anti-virus software that increased file opening delays.

[sky, 2015-10-23 16:01:07]

Description was changed from

==========
Load the table of visited links from database file asynchronously.

Remove access to the database file from the UI thread and reduce
startup browser delays.

R=sky@chromium.org
BUG=24163
BUG=45591
==========

to

==========
Load the table of visited links from database file asynchronously.

Remove access to the database file from the UI thread and reduce
startup browser delays.

R=sky@chromium.org
BUG=24163
BUG=45591
==========

[sky, 2015-10-23 16:01:07]

sky@chromium.org changed reviewers:
+ brettw@chromium.org

[sky, 2015-10-23 16:01:18]

Wow, nice improvement.
I'm going to bounce this one to Brett.

[brettw, 2015-11-02 05:09:23]

Thanks! Sorry for the delay in reviewing, I was out. I'm also not sure about my
schedule this week, it may be difficult for me to reply right away but I will
definitely have time next week.

https://codereview.chromium.org/1417943002/diff/1/components/visitedlink/brow...
File components/visitedlink/browser/visitedlink_master.cc (right):

https://codereview.chromium.org/1417943002/diff/1/components/visitedlink/brow...
components/visitedlink/browser/visitedlink_master.cc:678: // was cleared.
"was" -> "could have been" (As-is, I think the comment describes the "return"
case rather than the entire block fo code here, which I'm trying to make clear.)

https://codereview.chromium.org/1417943002/diff/1/components/visitedlink/brow...
components/visitedlink/browser/visitedlink_master.cc:731: for (const auto&
fingerprint : added_since_rebuild_or_load_)
From here to the end of the condition is the same as OnTableRebuildComplete. I
think this is probably complex enough to warrant a helper function to avoid the
duplication.

https://codereview.chromium.org/1417943002/diff/1/components/visitedlink/brow...
components/visitedlink/browser/visitedlink_master.cc:743: // Send an update
notification to all child processes.
This will duplicate the message if the table is resized above. I'm not actually
sure if this is benign or not. If it doesn't hurt, this is a rare edge case and
I don't think it's worth optimizing. But I'm concerned about handle duplicating
behavior on all the platforms when sharing the same memory to a process multiple
times. For example, the renderer will close the old one and add the new one, but
I'm not sure the two handles to the same memory will always be different on all
platforms.

I think this will all go away if we move this NewTable call above the "if
(!added_since..." condition above, and then also delete the checking for whether
the new size is required at the top of that condition. The additions will
organically cause a table resize if necessary and we can have fewer special
cases here. I think the only case where manually checking for the resize might
help is if the additions cause two resizes, which should be so rare as to be
almost impossible.

https://codereview.chromium.org/1417943002/diff/1/components/visitedlink/brow...
components/visitedlink/browser/visitedlink_master.cc:744:
listener_->NewTable(shared_memory_);
I *think* here we also need a Reset() after the NewTable. You may have this in
your head more freshly than me, so please correct if I'm mistaken...

Now that we're loading the links asynchronously, the new table means the visited
link state of all renderers is out-of-date because we loaded all of the hashes
from disk without sending notifications. I think this only happens in Reset().
The NewTable() assumes the contents are the same but the table was expanded, so
isn't sufficient, but obviously the NewTable call has to happen first for the
new link state to be updated. Otherwise the link state on the initial tabs will
be incorrect.

If we move NewTable up above, the Reset() needs to stay at the bottom.

[sath, 2015-11-18 12:18:45]

Hello! Sorry for my English. And sorry for keep waiting for my next part fix.
On 2015/11/02 05:09:23, brettw wrote:
> Thanks! Sorry for the delay in reviewing, I was out. I'm also not sure about
my
> schedule this week, it may be difficult for me to reply right away but I will
> definitely have time next week.
> 
>
https://codereview.chromium.org/1417943002/diff/1/components/visitedlink/brow...
> File components/visitedlink/browser/visitedlink_master.cc (right):
> 
>
https://codereview.chromium.org/1417943002/diff/1/components/visitedlink/brow...
> components/visitedlink/browser/visitedlink_master.cc:678: // was cleared.
> "was" -> "could have been" (As-is, I think the comment describes the "return"
> case rather than the entire block fo code here, which I'm trying to make
clear.)
I fixed the comment.
> 
>
https://codereview.chromium.org/1417943002/diff/1/components/visitedlink/brow...
> components/visitedlink/browser/visitedlink_master.cc:731: for (const auto&
> fingerprint : added_since_rebuild_or_load_)
> From here to the end of the condition is the same as OnTableRebuildComplete. I
> think this is probably complex enough to warrant a helper function to avoid
the
> duplication.
I made a mistake. After loading the table we always change the salt and we
couldn't 
add/remove accumulated fingerprints to/from the loaded table. Now for that
purpose I use temporary sets with urls. Also after loading the table we need to
inform render processes to drop cached visited link hashes and refresh visible
state
of links.
> 
>
https://codereview.chromium.org/1417943002/diff/1/components/visitedlink/brow...
> components/visitedlink/browser/visitedlink_master.cc:743: // Send an update
> notification to all child processes.
> This will duplicate the message if the table is resized above. I'm not
actually
> sure if this is benign or not. If it doesn't hurt, this is a rare edge case
and
> I don't think it's worth optimizing. But I'm concerned about handle
duplicating
> behavior on all the platforms when sharing the same memory to a process
multiple
> times. For example, the renderer will close the old one and add the new one,
but
> I'm not sure the two handles to the same memory will always be different on
all
> platforms.
> 
> I think this will all go away if we move this NewTable call above the "if
> (!added_since..." condition above, and then also delete the checking for
whether
> the new size is required at the top of that condition. The additions will
> organically cause a table resize if necessary and we can have fewer special
> cases here. I think the only case where manually checking for the resize might
> help is if the additions cause two resizes, which should be so rare as to be
> almost impossible.
> 
>
https://codereview.chromium.org/1417943002/diff/1/components/visitedlink/brow...
> components/visitedlink/browser/visitedlink_master.cc:744:
> listener_->NewTable(shared_memory_);
> I *think* here we also need a Reset() after the NewTable. You may have this in
> your head more freshly than me, so please correct if I'm mistaken...
> 
> Now that we're loading the links asynchronously, the new table means the
visited
> link state of all renderers is out-of-date because we loaded all of the hashes
> from disk without sending notifications. I think this only happens in Reset().
> The NewTable() assumes the contents are the same but the table was expanded,
so
> isn't sufficient, but obviously the NewTable call has to happen first for the
> new link state to be updated. Otherwise the link state on the initial tabs
will
> be incorrect.
> 
> If we move NewTable up above, the Reset() needs to stay at the bottom.
I've moved NewTable above condition and rewritten condition about resizing the
table.

[sath, 2015-12-03 17:14:35]

Hello! Please take look at my fixes.

[brettw, 2015-12-03 23:08:46]

Ah, sorry for losing this! I will look tomorrow. In the future, please feel free
to ping me more often instead of waiting 2 weeks (which is really too long to
make you wait).

[brettw, 2015-12-08 00:54:42]

I have some minor comments, but nothing blocking, so LGTM. As I mention below,
you should get a blink reviewer to double-check the code in dom/ even if my
review makes the presubmit bot happy.

Sorry again for taking so long on this! This change seems like a positive
improvement.

https://codereview.chromium.org/1417943002/diff/20001/components/visitedlink/...
File components/visitedlink/browser/visitedlink_master.cc (right):

https://codereview.chromium.org/1417943002/diff/20001/components/visitedlink/...
components/visitedlink/browser/visitedlink_master.cc:711: // This temprorary
sets are needed only to rebuild table.
Spelling: temporary

https://codereview.chromium.org/1417943002/diff/20001/components/visitedlink/...
File components/visitedlink/browser/visitedlink_master.h (right):

https://codereview.chromium.org/1417943002/diff/20001/components/visitedlink/...
components/visitedlink/browser/visitedlink_master.h:62: // the table was rebuilt
or loaded.
Can this comment explain at a higher-level explicitly what invalidate_hashes
means? I assume this is because the salt can change and everything needs to be
recomputed.

https://codereview.chromium.org/1417943002/diff/20001/components/visitedlink/...
components/visitedlink/browser/visitedlink_master.h:316:
VisitedLinkCommon::Fingerprint** hash_table);
In the comment above, can you make more explicit how this works, especially for
memory management.

https://codereview.chromium.org/1417943002/diff/20001/components/visitedlink/...
File components/visitedlink/common/visitedlink_messages.h (right):

https://codereview.chromium.org/1417943002/diff/20001/components/visitedlink/...
components/visitedlink/common/visitedlink_messages.h:28: // cached visited links
hashes.
Whatever you add to visitedlink_master.h about hash updating, can you add here
as well?

https://codereview.chromium.org/1417943002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/VisitedLinkState.cpp (right):

https://codereview.chromium.org/1417943002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/VisitedLinkState.cpp:68: if
(invalidateVisitedLinkHashes && isHTMLAnchorElement(node))
Although I'm in the root-level owners file, I'm not really qualified to review
this code. Can you get a WebKit/DOM reviewer to look at this? I think it's all
trivial except for this one function, which is hopefully also trivial from their
perspective.

[sath, 2015-12-08 09:41:10]

sath@yandex-team.ru changed reviewers:
+ dglazkov@chromium.org

[sath, 2015-12-09 11:00:19]

Hello! I have added some comments in places that you pointed out.
And now I am waiting for review of WebKit/DOM part of the patch.

[brettw, 2015-12-10 18:43:52]

dglazkov is usually pretty overloaded. I'd find another owner in Source/core to
review this. I think it's OK if that person isn't also an owner of public and
web, since those things are a trivial extension of the core change, and my
review should make the bot happy.

[sath, 2015-12-10 18:55:39]

sath@yandex-team.ru changed reviewers:
+ rbyers@chromium.org
- dglazkov@chromium.org

[sath, 2015-12-10 19:00:33]

I have changed dglazkov to rbyers? Is it okay?
On 2015/12/10 18:43:52, brettw wrote:
> dglazkov is usually pretty overloaded. I'd find another owner in Source/core
to
> review this. I think it's OK if that person isn't also an owner of public and
> web, since those things are a trivial extension of the core change, and my
> review should make the bot happy.

[brettw, 2015-12-10 21:59:08]

On 2015/12/10 19:00:33, sath wrote:
> I have changed dglazkov to rbyers? Is it okay?

As far as I know this is probably a better bet.

[Rick Byers, 2015-12-10 22:24:48]

rbyers@chromium.org changed reviewers:
+ tkent@chromium.org

[Rick Byers, 2015-12-10 22:24:48]

WebKit/public/ and WebKit/web LGTM with nit

But I really have no context with how visited links are tracked in blink, you
probably want someone from the DOM team to review the changes in core.  +tkent
perhaps.

https://codereview.chromium.org/1417943002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebView.h (right):

https://codereview.chromium.org/1417943002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebView.h:427: BLINK_EXPORT static void
resetVisitedLinkState(bool invalidateVisitedLinkHashes);
nit: please add a comment describing what the exact semantics of this bool are. 
We want the blink API to be pretty well documented.

[tkent, 2015-12-10 23:47:27]

Do all layout tests pass with this CL?
Are visited <a> elements correctly repainted after the db loading?

[sath, 2015-12-14 13:52:48]

On 2015/12/10 23:47:27, tkent wrote:
> Do all layout tests pass with this CL?
> Are visited <a> elements correctly repainted after the db loading?

Hello!
I have checked locally layout tests which included substring 'visited'
and all of them passed.

But I found unused file
'/third_party/WebKit/LayoutTests/fast/history/resources/clicked-link-is-visited-2.html'.
It was added with commit 1a34ed78f112ea80a38f8a7d43c23772ed0d7a76 with
other file 'fast/history/clicked-link-is-visited.html'.
'clicked-link-is-visited.html'
included 'clicked-link-is-visited-2.html'. 
'clicked-link-is-visited.html' was deleted in commit
5b75261945c63af9753daf57517bfec7aa007d74
But 'clicked-link-is-visited-2.html' is still here and now it isn't used.

[tkent, 2015-12-14 21:38:47]

The CQ bit was checked by tkent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-14 21:39:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1417943002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1417943002/40001

[commit-bot: I haz the power, 2015-12-14 23:08:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-14 23:08:20]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tkent, 2015-12-14 23:39:28]

lgtm

[sath, 2015-12-15 11:27:38]

The CQ bit was checked by sath@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2015-12-15 11:27:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1417943002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1417943002/60001

[commit-bot: I haz the power, 2015-12-15 12:49:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-15 12:49:54]

Dry run: This issue passed the CQ dry run.

[sath, 2015-12-15 14:31:45]

The CQ bit was checked by sath@yandex-team.ru

[sath, 2015-12-15 14:31:46]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
tkent@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1417943002/#ps60001
(title: "Add extra comment and fix compilation error.")

[commit-bot: I haz the power, 2015-12-15 14:32:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1417943002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1417943002/60001

[commit-bot: I haz the power, 2015-12-15 14:40:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-15 14:40:29]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sath, 2015-12-15 14:55:22]

sath@yandex-team.ru changed reviewers:
+ kenrb@chromium.org
- sky@chromium.org

[sath, 2015-12-15 15:00:33]

I need to additional LGTM for file visitedlink_messages.h.

On 2015/12/15 14:40:29, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[kenrb, 2015-12-15 15:04:23]

ipc lgtm

[sath, 2015-12-15 15:05:02]

The CQ bit was checked by sath@yandex-team.ru

[commit-bot: I haz the power, 2015-12-15 15:05:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1417943002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1417943002/60001

[commit-bot: I haz the power, 2015-12-15 15:10:52]

Description was changed from

==========
Load the table of visited links from database file asynchronously.

Remove access to the database file from the UI thread and reduce
startup browser delays.

R=sky@chromium.org
BUG=24163
BUG=45591
==========

to

==========
Load the table of visited links from database file asynchronously.

Remove access to the database file from the UI thread and reduce
startup browser delays.

R=sky@chromium.org
BUG=24163
BUG=45591
==========

[commit-bot: I haz the power, 2015-12-15 15:10:53]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-15 15:11:58]

Description was changed from

==========
Load the table of visited links from database file asynchronously.

Remove access to the database file from the UI thread and reduce
startup browser delays.

R=sky@chromium.org
BUG=24163
BUG=45591
==========

to

==========
Load the table of visited links from database file asynchronously.

Remove access to the database file from the UI thread and reduce
startup browser delays.

R=sky@chromium.org
BUG=24163
BUG=45591

Committed: https://crrev.com/af37203df5ddaf2b1491748ca4c0428a415e0f88
Cr-Commit-Position: refs/heads/master@{#365250}
==========

[commit-bot: I haz the power, 2015-12-15 15:11:59]

Patchset 4 (id:??) landed as
https://crrev.com/af37203df5ddaf2b1491748ca4c0428a415e0f88
Cr-Commit-Position: refs/heads/master@{#365250}