Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2158123003:
[MD Settings] Remove jank for webui. (Closed)
|
Created:
4 years, 5 months ago by groby-ooo-7-16 Modified:
4 years, 4 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MD Settings] Remove jank for webui.
WebUI URLRequest source data is (on desktop) backed by a memory-mapped
file. Copying that data in CompleteRead can trigger jankiness due to
potential file IO caused by the read.
This CL moves the memcpy on the file thread.
R=dbeam@chromium.org
BUG=455423
Committed: https://crrev.com/b6b53857a5ad6aecc8752bccb947147d101d8417
Cr-Commit-Position: refs/heads/master@{#407698}
Patch Set 1 #Patch Set 2 : Do all the things on a worker thread. #
Total comments: 22
Patch Set 3 : Review fixes. #
Total comments: 5
Dependent Patchsets: Messages
Total messages: 35 (16 generated)
The CQ bit was checked by groby@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by groby@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL
disclaimer: I'm not terrrribly familiar with this code /cc mmenke@ https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... File content/browser/webui/url_data_manager_backend.cc (right): https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:114: if (buf_size > 0) { when will this ever be == 0? https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:288: task_runner_(base::WorkerPool::GetTaskRunner(false)), do we want to ref the workerpool the whole time this class exists? https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:431: can we just do if (buf_size == 0) { task_runner_ = nullptr; return 0; } // maybe if (!task_runner_) task_runner_ = base::WorkerPool::GetTaskRunner(false); task_runner_->PostTaskAndReply(...)
I'll plan to do a pass tomorrow.
https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... File content/browser/webui/url_data_manager_backend.cc (right): https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:114: if (buf_size > 0) { On 2016/07/19 23:27:33, Dan Beam wrote: > when will this ever be == 0? It used to be possible in a previous iteration of the code :) (Removing) https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:288: task_runner_(base::WorkerPool::GetTaskRunner(false)), On 2016/07/19 23:27:33, Dan Beam wrote: > do we want to ref the workerpool the whole time this class exists? We'll need it at the very least until all read requests are satisfied. That's pretty much the entire duration of the job's life. Either way, it's not really a difference because the task runners are a global - it doesn't matter how long we hold them : ) (And this way, this is closer to URLRequestSimpleJob, which I'd _like_ to combine with this at some point) https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:431: On 2016/07/19 23:27:33, Dan Beam wrote: > can we just do > > if (buf_size == 0) { > task_runner_ = nullptr; > return 0; > } > > // maybe if (!task_runner_) > task_runner_ = base::WorkerPool::GetTaskRunner(false); > task_runner_->PostTaskAndReply(...) We could - but there's no benefit to it. See above on lifetime. (Plus, as above, using the SimpleJob looms. We're almost there, but not quite. See bug for discussion why I can't use it yet)
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
This seems reasonable to me, just some minor suggestions https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... File content/browser/webui/url_data_manager_backend.cc (right): https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:288: task_runner_(base::WorkerPool::GetTaskRunner(false)), On 2016/07/20 01:01:40, groby wrote: > On 2016/07/19 23:27:33, Dan Beam wrote: > > do we want to ref the workerpool the whole time this class exists? > > We'll need it at the very least until all read requests are satisfied. That's > pretty much the entire duration of the job's life. > > Either way, it's not really a difference because the task runners are a global - > it doesn't matter how long we hold them : ) (And this way, this is closer to > URLRequestSimpleJob, which I'd _like_ to combine with this at some point) You don't need to hold onto it - it's a leaked global. You can just inline base::WorkerPool::GetTaskRunner wherever you need to use it. https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:387: // If we don't have data, signal an error. Here and below, some people hold pretty strongly to avoiding "we" in comments, since it's ambiguous ("we" the chromium authors? "we" the class? etc). Up to you if you want to reword, just thought I'd mention it. https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:396: if (pending_buf_.get()) { optioanl nit: .get() not needed. (I feel if it's worth the effort of making scoped_refptrs work in conditionals without the .get, then it should be the preferred pattern) https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:421: int URLRequestChromeJob::PostReadTask(net::IOBuffer* buf, int buf_size) { Seems a little weird to take bug and buf_size as arguments, since they're class variables. I'd suggest just unconditionally setting them in ReadRawData, and then making this take no arguments. Makes both paths more uniform. Would also need to clear pending_buf_ in this method, instead of in DataAvailable. https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:422: DCHECK(buf && data_); Should split this into two DCHECKs, so if one fails, it's clearer which. https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:432: task_runner_->PostTaskAndReply( Could just use WorkerPool::PostTaskAndReply, and get rid of the TaskRunner.
https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... File content/browser/webui/url_data_manager_backend.cc (right): https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:288: task_runner_(base::WorkerPool::GetTaskRunner(false)), On 2016/07/20 16:47:58, mmenke wrote: > On 2016/07/20 01:01:40, groby wrote: > > On 2016/07/19 23:27:33, Dan Beam wrote: > > > do we want to ref the workerpool the whole time this class exists? > > > > We'll need it at the very least until all read requests are satisfied. That's > > pretty much the entire duration of the job's life. > > > > Either way, it's not really a difference because the task runners are a global > - > > it doesn't matter how long we hold them : ) (And this way, this is closer to > > URLRequestSimpleJob, which I'd _like_ to combine with this at some point) > > You don't need to hold onto it - it's a leaked global. You can just inline > base::WorkerPool::GetTaskRunner wherever you need to use it. Sounds good. https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:387: // If we don't have data, signal an error. On 2016/07/20 16:47:58, mmenke wrote: > Here and below, some people hold pretty strongly to avoiding "we" in comments, > since it's ambiguous ("we" the chromium authors? "we" the class? etc). Up to > you if you want to reword, just thought I'd mention it. Clearly, it's the royal "we" ;) Fixed https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:396: if (pending_buf_.get()) { On 2016/07/20 16:47:58, mmenke wrote: > optioanl nit: .get() not needed. (I feel if it's worth the effort of making > scoped_refptrs work in conditionals without the .get, then it should be the > preferred pattern) Thanks for flagging, fixed. https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:421: int URLRequestChromeJob::PostReadTask(net::IOBuffer* buf, int buf_size) { On 2016/07/20 16:47:58, mmenke wrote: > Seems a little weird to take bug and buf_size as arguments, since they're class > variables. I'd suggest just unconditionally setting them in ReadRawData, and > then making this take no arguments. Makes both paths more uniform. > > Would also need to clear pending_buf_ in this method, instead of in > DataAvailable. That was my original solution - I switched to this for three reasons: 1) The current setup makes it slightly clearer (to me?) that pending_buf_ really is only satisfying the first request - after that, all requests are satisfied immediately. 2) It mirrors the ReadRawData API, which is fitting, since ReadRawData just posts its params to a task. 3) I have a dislike for functions that use object state to pass parameters. Yes, I'm still using |data_|, but that is state that is actually modified. The buffer is only state as long as it's a pending buffer due to data not being available. If you'd like me to switch over anyways, please let me know. https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:422: DCHECK(buf && data_); On 2016/07/20 16:47:58, mmenke wrote: > Should split this into two DCHECKs, so if one fails, it's clearer which. Done. https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:432: task_runner_->PostTaskAndReply( On 2016/07/20 16:47:58, mmenke wrote: > Could just use WorkerPool::PostTaskAndReply, and get rid of the TaskRunner. I looked at it, but it creates two additional objects (PostTaskAndReplyWorkerPool on the stack, PostTaskAndReplyRelay on the heap), and it involves posting an additional task (PostTaskAndReplyRelay::Run, which will then do the actual PostTaskAndReply), presumably increasing latency. And I _really_ would like to keep this as low latency as possible. (You'd think it doesn't matter, but md-settings loads a _lot_ of resources, in a chained way. It adds up) Again, if I'm missing something or am overly cautious, please LMK and I'll fix.
The CQ bit was checked by groby@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM. Some responses and another comment, but it's fine as-is. https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... File content/browser/webui/url_data_manager_backend.cc (right): https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:421: int URLRequestChromeJob::PostReadTask(net::IOBuffer* buf, int buf_size) { On 2016/07/20 18:10:11, groby wrote: > On 2016/07/20 16:47:58, mmenke wrote: > > Seems a little weird to take bug and buf_size as arguments, since they're > class > > variables. I'd suggest just unconditionally setting them in ReadRawData, and > > then making this take no arguments. Makes both paths more uniform. > > > > Would also need to clear pending_buf_ in this method, instead of in > > DataAvailable. > > That was my original solution - I switched to this for three reasons: > > 1) The current setup makes it slightly clearer (to me?) that pending_buf_ really > is only satisfying the first request - after that, all requests are satisfied > immediately. > 2) It mirrors the ReadRawData API, which is fitting, since ReadRawData just > posts its params to a task. > 3) I have a dislike for functions that use object state to pass parameters. Yes, > I'm still using |data_|, but that is state that is actually modified. The buffer > is only state as long as it's a pending buffer due to data not being available. > > If you'd like me to switch over anyways, please let me know. I don't have a particularly strong opinion here, just feel that making both paths as similar as possible is the least likely to lead to problems down the line (And you're using the class variables to pass parameters to DataAvailable from here, already). If you think this is clearer, happy to defer to you. https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:432: task_runner_->PostTaskAndReply( On 2016/07/20 18:10:11, groby wrote: > On 2016/07/20 16:47:58, mmenke wrote: > > Could just use WorkerPool::PostTaskAndReply, and get rid of the TaskRunner. > > I looked at it, but it creates two additional objects > (PostTaskAndReplyWorkerPool on the stack, PostTaskAndReplyRelay on the heap), > and it involves posting an additional task (PostTaskAndReplyRelay::Run, which > will then do the actual PostTaskAndReply), presumably increasing latency. > > And I _really_ would like to keep this as low latency as possible. (You'd think > it doesn't matter, but md-settings loads a _lot_ of resources, in a chained way. > It adds up) > > Again, if I'm missing something or am overly cautious, please LMK and I'll fix. Code size generally matters more, and the single call seems better, from that standpoint. If WorkerPool::PostTaskAndReply is really slower, wouldn't it make more sense to modify WorkerPool::PostTaskAndReply itself to just be: GetTaskRunner(task_is_slow)->PostTaskAndReply(from_here, task, reply);? I have no idea why it currently doesn't just do this. https://codereview.chromium.org/2158123003/diff/40001/content/browser/webui/u... File content/browser/webui/url_data_manager_backend.cc (right): https://codereview.chromium.org/2158123003/diff/40001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:429: base::WorkerPool::GetTaskRunner(false)->PostTaskAndReply( I think this should be true? Potential disk accesses are considered slow, I assume.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm but consider classifying this as slow if it hits disk as mmenke@ has said here's the only difference if you mark something as slow: https://cs.chromium.org/chromium/src/base/threading/worker_pool_win.cc?dr=CSs... it currently makes use of WT_EXECUTELONGFUNCTION on windows-only (may change, though). An article about what this means: https://blogs.msdn.microsoft.com/oldnewthing/20050722-15/?p=34843 tl;dr - I don't really care either way, but it may cause another thread to be created on Windows.
Will land as-is, but have a potential follow-up on which worker pool to use. (Request is on chromium-dev) https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... File content/browser/webui/url_data_manager_backend.cc (right): https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:421: int URLRequestChromeJob::PostReadTask(net::IOBuffer* buf, int buf_size) { On 2016/07/20 18:19:43, mmenke wrote: > On 2016/07/20 18:10:11, groby wrote: > > On 2016/07/20 16:47:58, mmenke wrote: > > > Seems a little weird to take bug and buf_size as arguments, since they're > > class > > > variables. I'd suggest just unconditionally setting them in ReadRawData, > and > > > then making this take no arguments. Makes both paths more uniform. > > > > > > Would also need to clear pending_buf_ in this method, instead of in > > > DataAvailable. > > > > That was my original solution - I switched to this for three reasons: > > > > 1) The current setup makes it slightly clearer (to me?) that pending_buf_ > really > > is only satisfying the first request - after that, all requests are satisfied > > immediately. > > 2) It mirrors the ReadRawData API, which is fitting, since ReadRawData just > > posts its params to a task. > > 3) I have a dislike for functions that use object state to pass parameters. > Yes, > > I'm still using |data_|, but that is state that is actually modified. The > buffer > > is only state as long as it's a pending buffer due to data not being > available. > > > > If you'd like me to switch over anyways, please let me know. > > I don't have a particularly strong opinion here, just feel that making both > paths as similar as possible is the least likely to lead to problems down the > line (And you're using the class variables to pass parameters to DataAvailable > from here, already). If you think this is clearer, happy to defer to you. Acknowledged. https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:432: task_runner_->PostTaskAndReply( On 2016/07/20 18:19:43, mmenke wrote: > On 2016/07/20 18:10:11, groby wrote: > > On 2016/07/20 16:47:58, mmenke wrote: > > > Could just use WorkerPool::PostTaskAndReply, and get rid of the TaskRunner. > > > > I looked at it, but it creates two additional objects > > (PostTaskAndReplyWorkerPool on the stack, PostTaskAndReplyRelay on the heap), > > and it involves posting an additional task (PostTaskAndReplyRelay::Run, which > > will then do the actual PostTaskAndReply), presumably increasing latency. > > > > And I _really_ would like to keep this as low latency as possible. (You'd > think > > it doesn't matter, but md-settings loads a _lot_ of resources, in a chained > way. > > It adds up) > > > > Again, if I'm missing something or am overly cautious, please LMK and I'll > fix. > > Code size generally matters more, and the single call seems better, from that > standpoint. > > If WorkerPool::PostTaskAndReply is really slower, wouldn't it make more sense to > modify WorkerPool::PostTaskAndReply itself to just be: > > GetTaskRunner(task_is_slow)->PostTaskAndReply(from_here, task, reply);? Digging deeper, the relay is necessary anyways, and so is the intermediate workerpool. The task_runner_ call does the same thing, just in different places. Oh, well, at least I learned a bit more about our infrastructure :) Anyways, changed and Done. > > I have no idea why it currently doesn't just do this. https://codereview.chromium.org/2158123003/diff/40001/content/browser/webui/u... File content/browser/webui/url_data_manager_backend.cc (right): https://codereview.chromium.org/2158123003/diff/40001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:429: base::WorkerPool::GetTaskRunner(false)->PostTaskAndReply( On 2016/07/20 18:19:44, mmenke wrote: > I think this should be true? Potential disk accesses are considered slow, I > assume. I'm basing this on URLRequestSimpleJob - that also uses false. I've asked chromium-dev, but no answer so far. I'll leave it for now.
The CQ bit was checked by groby@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/26 02:37:45, groby wrote: > Will land as-is, but have a potential follow-up on which worker pool to use. > (Request is on chromium-dev) > > https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... > File content/browser/webui/url_data_manager_backend.cc (right): > > https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... > content/browser/webui/url_data_manager_backend.cc:421: int > URLRequestChromeJob::PostReadTask(net::IOBuffer* buf, int buf_size) { > On 2016/07/20 18:19:43, mmenke wrote: > > On 2016/07/20 18:10:11, groby wrote: > > > On 2016/07/20 16:47:58, mmenke wrote: > > > > Seems a little weird to take bug and buf_size as arguments, since they're > > > class > > > > variables. I'd suggest just unconditionally setting them in ReadRawData, > > and > > > > then making this take no arguments. Makes both paths more uniform. > > > > > > > > Would also need to clear pending_buf_ in this method, instead of in > > > > DataAvailable. > > > > > > That was my original solution - I switched to this for three reasons: > > > > > > 1) The current setup makes it slightly clearer (to me?) that pending_buf_ > > really > > > is only satisfying the first request - after that, all requests are > satisfied > > > immediately. > > > 2) It mirrors the ReadRawData API, which is fitting, since ReadRawData just > > > posts its params to a task. > > > 3) I have a dislike for functions that use object state to pass parameters. > > Yes, > > > I'm still using |data_|, but that is state that is actually modified. The > > buffer > > > is only state as long as it's a pending buffer due to data not being > > available. > > > > > > If you'd like me to switch over anyways, please let me know. > > > > I don't have a particularly strong opinion here, just feel that making both > > paths as similar as possible is the least likely to lead to problems down the > > line (And you're using the class variables to pass parameters to DataAvailable > > from here, already). If you think this is clearer, happy to defer to you. > > Acknowledged. > > https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... > content/browser/webui/url_data_manager_backend.cc:432: > task_runner_->PostTaskAndReply( > On 2016/07/20 18:19:43, mmenke wrote: > > On 2016/07/20 18:10:11, groby wrote: > > > On 2016/07/20 16:47:58, mmenke wrote: > > > > Could just use WorkerPool::PostTaskAndReply, and get rid of the > TaskRunner. > > > > > > I looked at it, but it creates two additional objects > > > (PostTaskAndReplyWorkerPool on the stack, PostTaskAndReplyRelay on the > heap), > > > and it involves posting an additional task (PostTaskAndReplyRelay::Run, > which > > > will then do the actual PostTaskAndReply), presumably increasing latency. > > > > > > And I _really_ would like to keep this as low latency as possible. (You'd > > think > > > it doesn't matter, but md-settings loads a _lot_ of resources, in a chained > > way. > > > It adds up) > > > > > > Again, if I'm missing something or am overly cautious, please LMK and I'll > > fix. > > > > Code size generally matters more, and the single call seems better, from that > > standpoint. > > > > If WorkerPool::PostTaskAndReply is really slower, wouldn't it make more sense > to > > modify WorkerPool::PostTaskAndReply itself to just be: > > > > GetTaskRunner(task_is_slow)->PostTaskAndReply(from_here, task, reply);? > > Digging deeper, the relay is necessary anyways, and so is the intermediate > workerpool. The task_runner_ call does the same thing, just in different places. > Oh, well, at least I learned a bit more about our infrastructure :) > > Anyways, changed and Done. > > > > > I have no idea why it currently doesn't just do this. > > https://codereview.chromium.org/2158123003/diff/40001/content/browser/webui/u... > File content/browser/webui/url_data_manager_backend.cc (right): > > https://codereview.chromium.org/2158123003/diff/40001/content/browser/webui/u... > content/browser/webui/url_data_manager_backend.cc:429: > base::WorkerPool::GetTaskRunner(false)->PostTaskAndReply( > On 2016/07/20 18:19:44, mmenke wrote: > > I think this should be true? Potential disk accesses are considered slow, I > > assume. > > I'm basing this on URLRequestSimpleJob - that also uses false. I've asked > chromium-dev, but no answer so far. I'll leave it for now. I'd say change them both to true in a followup, assuming the answer is that they should be considered slow (And I'm pretty sure they should be)
On 2016/07/26 02:46:54, mmenke wrote: > On 2016/07/26 02:37:45, groby wrote: > > Will land as-is, but have a potential follow-up on which worker pool to use. > > (Request is on chromium-dev) > > > > > https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... > > File content/browser/webui/url_data_manager_backend.cc (right): > > > > > https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... > > content/browser/webui/url_data_manager_backend.cc:421: int > > URLRequestChromeJob::PostReadTask(net::IOBuffer* buf, int buf_size) { > > On 2016/07/20 18:19:43, mmenke wrote: > > > On 2016/07/20 18:10:11, groby wrote: > > > > On 2016/07/20 16:47:58, mmenke wrote: > > > > > Seems a little weird to take bug and buf_size as arguments, since > they're > > > > class > > > > > variables. I'd suggest just unconditionally setting them in > ReadRawData, > > > and > > > > > then making this take no arguments. Makes both paths more uniform. > > > > > > > > > > Would also need to clear pending_buf_ in this method, instead of in > > > > > DataAvailable. > > > > > > > > That was my original solution - I switched to this for three reasons: > > > > > > > > 1) The current setup makes it slightly clearer (to me?) that pending_buf_ > > > really > > > > is only satisfying the first request - after that, all requests are > > satisfied > > > > immediately. > > > > 2) It mirrors the ReadRawData API, which is fitting, since ReadRawData > just > > > > posts its params to a task. > > > > 3) I have a dislike for functions that use object state to pass > parameters. > > > Yes, > > > > I'm still using |data_|, but that is state that is actually modified. The > > > buffer > > > > is only state as long as it's a pending buffer due to data not being > > > available. > > > > > > > > If you'd like me to switch over anyways, please let me know. > > > > > > I don't have a particularly strong opinion here, just feel that making both > > > paths as similar as possible is the least likely to lead to problems down > the > > > line (And you're using the class variables to pass parameters to > DataAvailable > > > from here, already). If you think this is clearer, happy to defer to you. > > > > Acknowledged. > > > > > https://codereview.chromium.org/2158123003/diff/20001/content/browser/webui/u... > > content/browser/webui/url_data_manager_backend.cc:432: > > task_runner_->PostTaskAndReply( > > On 2016/07/20 18:19:43, mmenke wrote: > > > On 2016/07/20 18:10:11, groby wrote: > > > > On 2016/07/20 16:47:58, mmenke wrote: > > > > > Could just use WorkerPool::PostTaskAndReply, and get rid of the > > TaskRunner. > > > > > > > > I looked at it, but it creates two additional objects > > > > (PostTaskAndReplyWorkerPool on the stack, PostTaskAndReplyRelay on the > > heap), > > > > and it involves posting an additional task (PostTaskAndReplyRelay::Run, > > which > > > > will then do the actual PostTaskAndReply), presumably increasing latency. > > > > > > > > And I _really_ would like to keep this as low latency as possible. (You'd > > > think > > > > it doesn't matter, but md-settings loads a _lot_ of resources, in a > chained > > > way. > > > > It adds up) > > > > > > > > Again, if I'm missing something or am overly cautious, please LMK and I'll > > > fix. > > > > > > Code size generally matters more, and the single call seems better, from > that > > > standpoint. > > > > > > If WorkerPool::PostTaskAndReply is really slower, wouldn't it make more > sense > > to > > > modify WorkerPool::PostTaskAndReply itself to just be: > > > > > > GetTaskRunner(task_is_slow)->PostTaskAndReply(from_here, task, reply);? > > > > Digging deeper, the relay is necessary anyways, and so is the intermediate > > workerpool. The task_runner_ call does the same thing, just in different > places. > > Oh, well, at least I learned a bit more about our infrastructure :) > > > > Anyways, changed and Done. > > > > > > > > I have no idea why it currently doesn't just do this. > > > > > https://codereview.chromium.org/2158123003/diff/40001/content/browser/webui/u... > > File content/browser/webui/url_data_manager_backend.cc (right): > > > > > https://codereview.chromium.org/2158123003/diff/40001/content/browser/webui/u... > > content/browser/webui/url_data_manager_backend.cc:429: > > base::WorkerPool::GetTaskRunner(false)->PostTaskAndReply( > > On 2016/07/20 18:19:44, mmenke wrote: > > > I think this should be true? Potential disk accesses are considered slow, I > > > assume. > > > > I'm basing this on URLRequestSimpleJob - that also uses false. I've asked > > chromium-dev, but no answer so far. I'll leave it for now. > > I'd say change them both to true in a followup, assuming the answer is that they > should be considered slow (And I'm pretty sure they should be) CL pending at https://codereview.chromium.org/2176373005 :)
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [MD Settings] Remove jank for webui. WebUI URLRequest source data is (on desktop) backed by a memory-mapped file. Copying that data in CompleteRead can trigger jankiness due to potential file IO caused by the read. This CL moves the memcpy on the file thread. R=dbeam@chromium.org BUG=455423 ========== to ========== [MD Settings] Remove jank for webui. WebUI URLRequest source data is (on desktop) backed by a memory-mapped file. Copying that data in CompleteRead can trigger jankiness due to potential file IO caused by the read. This CL moves the memcpy on the file thread. R=dbeam@chromium.org BUG=455423 Committed: https://crrev.com/b6b53857a5ad6aecc8752bccb947147d101d8417 Cr-Commit-Position: refs/heads/master@{#407698} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b6b53857a5ad6aecc8752bccb947147d101d8417 Cr-Commit-Position: refs/heads/master@{#407698}
Message was sent while issue was closed.
> I'd say change them both to true in a followup, assuming the answer is that they > should be considered slow (And I'm pretty sure they should be) Given the answer on chromium-dev (https://groups.google.com/a/chromium.org/d/msg/chromium-dev/8ih66Y7iXEo/ujJ0-...) it seems like leaving in the fast pool is OK. Does that work as an explanation for you, or should we pursue further?
Message was sent while issue was closed.
On 2016/07/26 20:48:11, groby wrote: > > I'd say change them both to true in a followup, assuming the answer is that > they > > should be considered slow (And I'm pretty sure they should be) > > Given the answer on chromium-dev > (https://groups.google.com/a/chromium.org/d/msg/chromium-dev/8ih66Y7iXEo/ujJ0-...) > it seems like leaving in the fast pool is OK. Does that work as an explanation > for you, or should we pursue further? I'll defer to the wisdom of the masses. I guess there was only one response. Maybe the wisdom of the guy who sounds like he knows what he's talking about?
Message was sent while issue was closed.
dproy@chromium.org changed reviewers: + dproy@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2158123003/diff/40001/content/browser/webui/u... File content/browser/webui/url_data_manager_backend.cc (right): https://codereview.chromium.org/2158123003/diff/40001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:396: pending_buf_ = nullptr; Drive-by comment since we had to revert this CL because of crbug.com/631812: I was interested in understanding better why this line was crashing chrome and it seems ReadRawDataComplete destroys the URLRequestJob, including its member variable pending_buf_. When we assign nullptr to pending_buf_ in the next line, it tries to release whatever pending_buf_ was pointing to, which now happens to be garbage memory, and so we get a segfault. Perhaps moving the nullptr assignment up two lines will fix the issue? This is how it worked before; the null assignment was done before ReadRawDataComplete. I don't know to full context of this CL so cannot say definitively if this is a proper fix, but I checked that this prevents the crash. Hope this helps.
Message was sent while issue was closed.
https://codereview.chromium.org/2158123003/diff/40001/content/browser/webui/u... File content/browser/webui/url_data_manager_backend.cc (right): https://codereview.chromium.org/2158123003/diff/40001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:396: pending_buf_ = nullptr; On 2016/07/27 18:15:23, dproy1 wrote: > Drive-by comment since we had to revert this CL because of crbug.com/631812: > I was interested in understanding better why this line was crashing chrome and > it seems ReadRawDataComplete destroys the URLRequestJob, including its member > variable pending_buf_. When we assign nullptr to pending_buf_ in the next line, > it tries to release whatever pending_buf_ was pointing to, which now happens to > be garbage memory, and so we get a segfault. > > Perhaps moving the nullptr assignment up two lines will fix the issue? This is > how it worked before; the null assignment was done before ReadRawDataComplete. > > I don't know to full context of this CL so cannot say definitively if this is a > proper fix, but I checked that this prevents the crash. Hope this helps. That does seem to be the issue. Finally investigating, thanks for the heads-up!
Message was sent while issue was closed.
https://codereview.chromium.org/2158123003/diff/40001/content/browser/webui/u... File content/browser/webui/url_data_manager_backend.cc (right): https://codereview.chromium.org/2158123003/diff/40001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:396: pending_buf_ = nullptr; On 2016/08/22 17:20:24, groby wrote: > On 2016/07/27 18:15:23, dproy1 wrote: > > Drive-by comment since we had to revert this CL because of crbug.com/631812: > > I was interested in understanding better why this line was crashing chrome and > > it seems ReadRawDataComplete destroys the URLRequestJob, including its member > > variable pending_buf_. When we assign nullptr to pending_buf_ in the next > line, > > it tries to release whatever pending_buf_ was pointing to, which now happens > to > > be garbage memory, and so we get a segfault. > > > > Perhaps moving the nullptr assignment up two lines will fix the issue? This is > > how it worked before; the null assignment was done before ReadRawDataComplete. > > > > > I don't know to full context of this CL so cannot say definitively if this is > a > > proper fix, but I checked that this prevents the crash. Hope this helps. > > That does seem to be the issue. Finally investigating, thanks for the heads-up! Hrm...Surely some of our browser tests run this code. Wonder why they didn't catch this. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
