Bind ModelLoader to a sequence, not a thread.

Bind ModelLoader to a sequence, not a thread.

BUG=714191
Review-Url: https://codereview.chromium.org/2831183004
Cr-Commit-Position: refs/heads/master@{#468252}
Committed: https://chromium.googlesource.com/chromium/src/+/d38126b6c8415249dd4a8d347d47c13d06831c0d

[Joe Mason, 2017-04-21 21:46:04]

joenotcharles@chromium.org changed reviewers:
+ gab@chromium.org

[gab, 2017-04-25 14:34:09]

Re. "not a thread" in CL desc. I don't see how it was previously bound to a
thread?

lgtm w/ comments, thanks!

https://codereview.chromium.org/2831183004/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/client_side_model_loader.cc (right):

https://codereview.chromium.org/2831183004/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_model_loader.cc:101: const std::string&
model_name)
Can avoid a string copy by taking a std::string by value and std::move()'ing it
into |name_| here :)

https://codereview.chromium.org/2831183004/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_model_loader.cc:110: // it doesn't have
to be the same one the ModelLoader is constructed on.
I typically call that the "creation sequence", i.e.

"but it doesn't have to be on the creation sequence."

https://codereview.chromium.org/2831183004/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_model_loader.cc:211: // called on the
same sequence because it invalidates the WeakPtr.
PS: ~ModelLoader() also invalidates WeakPtrs and should maybe have this check as
well? Unless you can guarantee destruction always occurs after completion ||
ModelLoader::CancelFetcher()? Destruction/construction sequence is usually the
same so it'll probably be hard to DCHECK on destruction. Either way WeakPtr's
DCHECK verify it's used properly (destroyed off-sequence only with no remaining
references from factory). In either case I like the explicit sequence checks
here for documentation :)

[Joe Mason, 2017-04-25 18:05:39]

On 2017/04/25 14:34:09, gab wrote:
> Re. "not a thread" in CL desc. I don't see how it was previously bound to a
> thread?

Just that the comment at the top of the file claimed it "expects all calls to be
made on the UI thread", which was never strictly true.

[Joe Mason, 2017-04-25 18:05:48]

https://codereview.chromium.org/2831183004/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/client_side_model_loader.cc (right):

https://codereview.chromium.org/2831183004/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_model_loader.cc:101: const std::string&
model_name)
On 2017/04/25 14:34:08, gab wrote:
> Can avoid a string copy by taking a std::string by value and std::move()'ing
it
> into |name_| here :)

I don't follow. Either we pass it in by reference, and it's copied when
assigning to name_, or it's copied during the pass-by-value.

https://codereview.chromium.org/2831183004/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_model_loader.cc:211: // called on the
same sequence because it invalidates the WeakPtr.
On 2017/04/25 14:34:08, gab wrote:
> PS: ~ModelLoader() also invalidates WeakPtrs and should maybe have this check
as
> well? Unless you can guarantee destruction always occurs after completion ||
> ModelLoader::CancelFetcher()? Destruction/construction sequence is usually the
> same so it'll probably be hard to DCHECK on destruction. Either way WeakPtr's
> DCHECK verify it's used properly (destroyed off-sequence only with no
remaining
> references from factory). In either case I like the explicit sequence checks
> here for documentation :)

Good catch. I'll double check on this before committing.

[gab, 2017-04-25 18:38:27]

https://codereview.chromium.org/2831183004/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/client_side_model_loader.cc (right):

https://codereview.chromium.org/2831183004/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_model_loader.cc:101: const std::string&
model_name)
On 2017/04/25 18:05:48, Joe Mason wrote:
> On 2017/04/25 14:34:08, gab wrote:
> > Can avoid a string copy by taking a std::string by value and std::move()'ing
> it
> > into |name_| here :)
> 
> I don't follow. Either we pass it in by reference, and it's copied when
> assigning to name_, or it's copied during the pass-by-value.

In the non-test constructor, it's constructed as a temporary string. Taking it
by reference here means that this temporary string is copied into |name_|
whereas moving the temporary into |name_| would avoid that.

[Joe Mason, 2017-04-28 13:54:18]

joenotcharles@chromium.org changed reviewers:
+ jialiul@chromium.org

[Joe Mason, 2017-04-28 13:54:19]

+jialul for OWNERS review. Do you have any opinions on the ModelLoader
destruction sequence we're discussing?

https://codereview.chromium.org/2831183004/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/client_side_model_loader.cc (right):

https://codereview.chromium.org/2831183004/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_model_loader.cc:101: const std::string&
model_name)
On 2017/04/25 18:38:27, gab wrote:
> On 2017/04/25 18:05:48, Joe Mason wrote:
> > On 2017/04/25 14:34:08, gab wrote:
> > > Can avoid a string copy by taking a std::string by value and
std::move()'ing
> > it
> > > into |name_| here :)
> > 
> > I don't follow. Either we pass it in by reference, and it's copied when
> > assigning to name_, or it's copied during the pass-by-value.
> 
> In the non-test constructor, it's constructed as a temporary string. Taking it
> by reference here means that this temporary string is copied into |name_|
> whereas moving the temporary into |name_| would avoid that.

But if I take it by value, wouldn't it be copied from the temporary to
|model_name|, and then moved from |model_name| to |name_|?

https://codereview.chromium.org/2831183004/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_model_loader.cc:211: // called on the
same sequence because it invalidates the WeakPtr.
On 2017/04/25 18:05:48, Joe Mason wrote:
> On 2017/04/25 14:34:08, gab wrote:
> > PS: ~ModelLoader() also invalidates WeakPtrs and should maybe have this
check
> as
> > well? Unless you can guarantee destruction always occurs after completion ||
> > ModelLoader::CancelFetcher()? Destruction/construction sequence is usually
the
> > same so it'll probably be hard to DCHECK on destruction. Either way
WeakPtr's
> > DCHECK verify it's used properly (destroyed off-sequence only with no
> remaining
> > references from factory). In either case I like the explicit sequence checks
> > here for documentation :)
> 
> Good catch. I'll double check on this before committing.

I think I'll remove the DetachFromSequence in the constructor, so that it
asserts construction, all WeakPtr-using methods, and destruction happen on the
same sequence. It's a simpler semantic even though technically a caller COULD
construct it on one sequence and then pass ownership of the ModelLoader to
another sequence, no callers do that now. What do you think?

[gab, 2017-04-28 16:29:53]

https://codereview.chromium.org/2831183004/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/client_side_model_loader.cc (right):

https://codereview.chromium.org/2831183004/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_model_loader.cc:101: const std::string&
model_name)
On 2017/04/28 13:54:19, Joe Mason wrote:
> On 2017/04/25 18:38:27, gab wrote:
> > On 2017/04/25 18:05:48, Joe Mason wrote:
> > > On 2017/04/25 14:34:08, gab wrote:
> > > > Can avoid a string copy by taking a std::string by value and
> std::move()'ing
> > > it
> > > > into |name_| here :)
> > > 
> > > I don't follow. Either we pass it in by reference, and it's copied when
> > > assigning to name_, or it's copied during the pass-by-value.
> > 
> > In the non-test constructor, it's constructed as a temporary string. Taking
it
> > by reference here means that this temporary string is copied into |name_|
> > whereas moving the temporary into |name_| would avoid that.
> 
> But if I take it by value, wouldn't it be copied from the temporary to
> |model_name|, and then moved from |model_name| to |name_|?

No because the temporary is an r-value and will be moved implicitly.

https://codereview.chromium.org/2831183004/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_model_loader.cc:211: // called on the
same sequence because it invalidates the WeakPtr.
On 2017/04/28 13:54:19, Joe Mason wrote:
> On 2017/04/25 18:05:48, Joe Mason wrote:
> > On 2017/04/25 14:34:08, gab wrote:
> > > PS: ~ModelLoader() also invalidates WeakPtrs and should maybe have this
> check
> > as
> > > well? Unless you can guarantee destruction always occurs after completion
||
> > > ModelLoader::CancelFetcher()? Destruction/construction sequence is usually
> the
> > > same so it'll probably be hard to DCHECK on destruction. Either way
> WeakPtr's
> > > DCHECK verify it's used properly (destroyed off-sequence only with no
> > remaining
> > > references from factory). In either case I like the explicit sequence
checks
> > > here for documentation :)
> > 
> > Good catch. I'll double check on this before committing.
> 
> I think I'll remove the DetachFromSequence in the constructor, so that it
> asserts construction, all WeakPtr-using methods, and destruction happen on the
> same sequence. It's a simpler semantic even though technically a caller COULD
> construct it on one sequence and then pass ownership of the ModelLoader to
> another sequence, no callers do that now. What do you think?

Yeah let's not detach in constructor if current users don't require it.

[Jialiu Lin, 2017-04-28 17:52:47]

https://codereview.chromium.org/2831183004/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/client_side_model_loader.cc (right):

https://codereview.chromium.org/2831183004/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_model_loader.cc:211: // called on the
same sequence because it invalidates the WeakPtr.
On 2017/04/28 at 16:29:52, gab wrote:
> On 2017/04/28 13:54:19, Joe Mason wrote:
> > On 2017/04/25 18:05:48, Joe Mason wrote:
> > > On 2017/04/25 14:34:08, gab wrote:
> > > > PS: ~ModelLoader() also invalidates WeakPtrs and should maybe have this
> > check
> > > as
> > > > well? Unless you can guarantee destruction always occurs after
completion ||
> > > > ModelLoader::CancelFetcher()? Destruction/construction sequence is
usually
> > the
> > > > same so it'll probably be hard to DCHECK on destruction. Either way
> > WeakPtr's
> > > > DCHECK verify it's used properly (destroyed off-sequence only with no
> > > remaining
> > > > references from factory). In either case I like the explicit sequence
checks
> > > > here for documentation :)
> > > 
> > > Good catch. I'll double check on this before committing.
> > 
> > I think I'll remove the DetachFromSequence in the constructor, so that it
> > asserts construction, all WeakPtr-using methods, and destruction happen on
the
> > same sequence. It's a simpler semantic even though technically a caller
COULD
> > construct it on one sequence and then pass ownership of the ModelLoader to
> > another sequence, no callers do that now. What do you think?
> 
> Yeah let's not detach in constructor if current users don't require it.

I'm OK with this. We are no longer actively changing this part of code. So a
simpler approach would be good enough.

[Joe Mason, 2017-04-28 19:43:55]

https://codereview.chromium.org/2831183004/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/client_side_model_loader.cc (right):

https://codereview.chromium.org/2831183004/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_model_loader.cc:101: const std::string&
model_name)
On 2017/04/28 16:29:53, gab wrote:
> On 2017/04/28 13:54:19, Joe Mason wrote:
> > On 2017/04/25 18:38:27, gab wrote:
> > > On 2017/04/25 18:05:48, Joe Mason wrote:
> > > > On 2017/04/25 14:34:08, gab wrote:
> > > > > Can avoid a string copy by taking a std::string by value and
> > std::move()'ing
> > > > it
> > > > > into |name_| here :)
> > > > 
> > > > I don't follow. Either we pass it in by reference, and it's copied when
> > > > assigning to name_, or it's copied during the pass-by-value.
> > > 
> > > In the non-test constructor, it's constructed as a temporary string.
Taking
> it
> > > by reference here means that this temporary string is copied into |name_|
> > > whereas moving the temporary into |name_| would avoid that.
> > 
> > But if I take it by value, wouldn't it be copied from the temporary to
> > |model_name|, and then moved from |model_name| to |name_|?
> 
> No because the temporary is an r-value and will be moved implicitly.

I see. Without the DetachFromSequence call there's not much point in the
combined constructor, though, so discarding this.

https://codereview.chromium.org/2831183004/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_model_loader.cc:110: // it doesn't have
to be the same one the ModelLoader is constructed on.
On 2017/04/25 14:34:08, gab wrote:
> I typically call that the "creation sequence", i.e.
> 
> "but it doesn't have to be on the creation sequence."

Removed the Detach call.

https://codereview.chromium.org/2831183004/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_model_loader.cc:211: // called on the
same sequence because it invalidates the WeakPtr.
On 2017/04/28 17:52:47, Jialiu Lin wrote:
> On 2017/04/28 at 16:29:52, gab wrote:
> > On 2017/04/28 13:54:19, Joe Mason wrote:
> > > On 2017/04/25 18:05:48, Joe Mason wrote:
> > > > On 2017/04/25 14:34:08, gab wrote:
> > > > > PS: ~ModelLoader() also invalidates WeakPtrs and should maybe have
this
> > > check
> > > > as
> > > > > well? Unless you can guarantee destruction always occurs after
> completion ||
> > > > > ModelLoader::CancelFetcher()? Destruction/construction sequence is
> usually
> > > the
> > > > > same so it'll probably be hard to DCHECK on destruction. Either way
> > > WeakPtr's
> > > > > DCHECK verify it's used properly (destroyed off-sequence only with no
> > > > remaining
> > > > > references from factory). In either case I like the explicit sequence
> checks
> > > > > here for documentation :)
> > > > 
> > > > Good catch. I'll double check on this before committing.
> > > 
> > > I think I'll remove the DetachFromSequence in the constructor, so that it
> > > asserts construction, all WeakPtr-using methods, and destruction happen on
> the
> > > same sequence. It's a simpler semantic even though technically a caller
> COULD
> > > construct it on one sequence and then pass ownership of the ModelLoader to
> > > another sequence, no callers do that now. What do you think?
> > 
> > Yeah let's not detach in constructor if current users don't require it.
> 
> I'm OK with this. We are no longer actively changing this part of code. So a
> simpler approach would be good enough. 

Done.

[Jialiu Lin, 2017-04-28 19:44:58]

lgtm

[gab, 2017-04-28 19:47:58]

lgtm

[Joe Mason, 2017-04-28 19:49:23]

The CQ bit was checked by joenotcharles@chromium.org

[commit-bot: I haz the power, 2017-04-28 19:50:18]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 20:01:15]

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

[commit-bot: I haz the power, 2017-04-28 20:01:16]

Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Joe Mason, 2017-04-30 03:08:00]

The CQ bit was checked by joenotcharles@chromium.org

[Joe Mason, 2017-04-30 03:08:00]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
jialiul@chromium.org
Link to the patchset: https://codereview.chromium.org/2831183004/#ps80001
(title: "Move WeakPtrFactory to end")

[commit-bot: I haz the power, 2017-04-30 03:08:07]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-30 03:54:20]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493521680545800,
"parent_rev": "a26ad25c4fd2cb7bbe286745d43e507dc658ba2c", "commit_rev":
"d38126b6c8415249dd4a8d347d47c13d06831c0d"}

[commit-bot: I haz the power, 2017-04-30 03:55:10]

Description was changed from

==========
Bind ModelLoader to a sequence, not a thread.

BUG=714191
==========

to

==========
Bind ModelLoader to a sequence, not a thread.

BUG=714191

Review-Url: https://codereview.chromium.org/2831183004
Cr-Commit-Position: refs/heads/master@{#468252}
Committed:
https://chromium.googlesource.com/chromium/src/+/d38126b6c8415249dd4a8d347d47...
==========

[commit-bot: I haz the power, 2017-04-30 03:55:11]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/d38126b6c8415249dd4a8d347d47...