Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2606263002:
Use background tasks for the compiler dispatcher (Closed)
|
Created:
3 years, 11 months ago by jochen (gone - plz use gerrit) Modified:
3 years, 11 months ago CC:
Michael Hablich, v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionUse background tasks for the compiler dispatcher
BUG=v8:5215
R=marja@chromium.org,vogelheim@chromium.org
Review-Url: https://codereview.chromium.org/2606263002
Cr-Commit-Position: refs/heads/master@{#42035}
Committed: https://chromium.googlesource.com/v8/v8/+/7a1b3a7bebbef88e72c9f7747e1930f10ee10c80
Patch Set 1 #
Total comments: 9
Patch Set 2 : unit tests #
Total comments: 40
Patch Set 3 : updates #
Total comments: 6
Patch Set 4 : updates #
Total comments: 6
Patch Set 5 : updates #
Messages
Total messages: 38 (21 generated)
The CQ bit was checked by jochen@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...
first attempt, ptal unit tests will come soon...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Generally looks good; some opinions below. I'm looking forward to the unit tests... :) https://codereview.chromium.org/2606263002/diff/1/src/compiler-dispatcher/com... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2606263002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:185: platform_->NumberOfAvailableBackgroundThreads() > 0; Generally speaking, it would be nice if the dispatcher could work on 'low-end' devices to exploit idle time, and high-end to exploit idle time + multiple cores. This looks like it will block the former. An alternative might be to check for platform_->NumberOfAvailableBackgroundThreads() > 0; in CanRunOnAnyThread That's admittedly a bit obtuse, since the platform isn't really a property of a job. Not sure it's worthwhile, though. Or if it's worthwhile now. https://codereview.chromium.org/2606263002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:195: pending_background_jobs_.erase(job); Do you need to update num_available_background_jobs_ here? https://codereview.chromium.org/2606263002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:343: } This logic is a bit weird, because first we take it out of the pending_background_jobs set, and then we might decide in the first if-branch to put it back in. No big deal, but.. weird. https://codereview.chromium.org/2606263002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:344: } [opinion ahead; feel free to ignore:] The can-idle or can-on-any-thread properties of the jobs don't really change... I wonder if it would be simpler to record those properties in the jobs, have a single mutex-locked queue of jobs, and then have the background and idle threads 'race' to pick available jobs of their respective type from that queue. (I think the current logic tries to handle this more intelligently by putting the different types of jobs into different queues/sets, but that 'distributes' the information across many data structures. I find that difficult to follow.) Related (and mostly arguing against my own proposal): It would be nice to have some logic to make this --predictable, other than disabling the scheduler entirely. The explicit assignment to queues probably makes that simpler. I'm not really sure, though.
https://codereview.chromium.org/2606263002/diff/1/src/compiler-dispatcher/com... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2606263002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:185: platform_->NumberOfAvailableBackgroundThreads() > 0; fair point. Since we only schedule background tasks up to NumberOfAvailableBackgroundThreads() I guess I could just drop that here https://codereview.chromium.org/2606263002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:195: pending_background_jobs_.erase(job); On 2017/01/02 at 17:16:24, vogelheim wrote: > Do you need to update num_available_background_jobs_ here? actually, I don't need that variable anymore, so I removed it https://codereview.chromium.org/2606263002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:344: } On 2017/01/02 at 17:16:24, vogelheim wrote: > [opinion ahead; feel free to ignore:] > > > The can-idle or can-on-any-thread properties of the jobs don't really change... I wonder if it would be simpler to record those properties in the jobs, have a single mutex-locked queue of jobs, and then have the background and idle threads 'race' to pick available jobs of their respective type from that queue. > > (I think the current logic tries to handle this more intelligently by putting the different types of jobs into different queues/sets, but that 'distributes' the information across many data structures. I find that difficult to follow.) > > > Related (and mostly arguing against my own proposal): It would be nice to have some logic to make this --predictable, other than disabling the scheduler entirely. The explicit assignment to queues probably makes that simpler. I'm not really sure, though. the can-idle property can change, when the estimates get updated. I don't think we can ever make this predictable, as idle time is just not predictable..
The CQ bit was checked by jochen@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...
ptal... now with tests
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/18494) v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)
lgtm, but w/ some more nitpicks & opinions. :) Also, two bots are unhappy... https://codereview.chromium.org/2606263002/diff/1/src/compiler-dispatcher/com... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2606263002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:344: } On 2017/01/02 19:39:39, jochen wrote: > On 2017/01/02 at 17:16:24, vogelheim wrote: > > [opinion ahead; feel free to ignore:] > > > > > > The can-idle or can-on-any-thread properties of the jobs don't really > change... I wonder if it would be simpler to record those properties in the > jobs, have a single mutex-locked queue of jobs, and then have the background and > idle threads 'race' to pick available jobs of their respective type from that > queue. > > > > (I think the current logic tries to handle this more intelligently by putting > the different types of jobs into different queues/sets, but that 'distributes' > the information across many data structures. I find that difficult to follow.) > > > > > > Related (and mostly arguing against my own proposal): It would be nice to have > some logic to make this --predictable, other than disabling the scheduler > entirely. The explicit assignment to queues probably makes that simpler. I'm not > really sure, though. > > the can-idle property can change, when the estimates get updated. > > I don't think we can ever make this predictable, as idle time is just not > predictable.. Hmm. Not sure I'm willing to give this up yet. For --predictable, we could implement a behaviour that it will always execute exactly one idle task regardless of size for every DoIdleWork call, in strict enqueing order. That'd be predictable, provided DoIdleWork calls are predictable. Then d8 could have a platform implementation that has idle callbacks based on a similarly predictable scheme. That'd give us some test coverage for the scheduler & the idle tasks, while (mostly?) preserving predictability. https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:201: DCHECK(pending_background_jobs_.find(job) == pending_background_jobs_.end()); Also the same DCHECK(...), with running_background_jobs? The post-condition is that job is in neither of those queues, right? https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:221: DCHECK(blocking == BlockingBehavior::kBlock); DCHECK_EQ ? https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:269: if (FLAG_single_threaded) return; Hmm. So if --single_threaded, ConsiderJobForBackgroundProcessing might put a job into pending_background_job_, and they'd just linger there until the main thread needs it and will FinishNow(job)? https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:307: ScheduleIdleTaskFromAnyThread(); ScheduleIdleTaskFromAnyThread unconditionally schedules the idle thread. Why do we know that we need one here? Is it because any background-compile always needs at least one main-thread step to finish? https://codereview.chromium.org/2606263002/diff/20001/test/unittests/compiler... File test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc (right): https://codereview.chromium.org/2606263002/diff/20001/test/unittests/compiler... test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:53: i::FLAG_ignition = true; Is this necessary? unittests/ uses gtest, which has a flag saver mechanism built in. Would this not cover this flag? https://codereview.chromium.org/2606263002/diff/20001/test/unittests/compiler... test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:97: idle_task_ = task; ASSERT_TRUE(idle_task_ == nullptr) before this line? https://codereview.chromium.org/2606263002/diff/20001/test/unittests/compiler... test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:116: bool IdleTaskPending() const { return !!idle_task_; } nitpick: The !! is a no-op here, since pointer-to-bool conversion is well defined in C++. (And we rely on it in every single if(ptr) .. statement.) https://codereview.chromium.org/2606263002/diff/20001/test/unittests/compiler... test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:138: for (auto&& task : tasks) { C++ question: Why the auto&& ? (I take it this makes no appreciable difference for auto resolving to a Task*; but I guess it's a defensive programming thing that should work better in some cases?) https://codereview.chromium.org/2606263002/diff/20001/test/unittests/compiler... test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:358: platform.RunIdleTask(1000.0, 0.0); [For my understanding:] What does this step accomplish? This test ultimately relies on the fact that a very expensive compile job will have exactly one background step?
lgtm, but... Like vogelheim@, I also find it confusing / surprising that the tasks don't get processed in any predictable order, and handling the jobs in the data structures is kinda weird... https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:190: void CompilerDispatcher::EnsureNotOnBackground(CompilerDispatcherJob* job) { This function name is confusing. I had to read it several times to understand what it does... So, what it does: if a job is currently running on background, it waits until it's done. "BlockIfJobIsRunningOnBackground"? "WaitForJobIfRunningOnBackground"? https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:339: auto it = pending_background_jobs_.find(job->second.get()); This is kinda weird... here we erase the job from pending_background_jobs and then we potentially insert it again if it's too big to complete now. So once we're running out of idle time, we might go through the whole pending_background_jobs_ structure, taking each job out and putting it back in. That feels weird... How about taking the job out of pending_background_jobs_ only if we're going to work on it? https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:352: ConsiderJobForBackgroundProcessing(job->second.get()); This is somewhat surprising... so we only push those jobs which are too long to complete (now) for background processing. Why is that? https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... File src/compiler-dispatcher/compiler-dispatcher.h (right): https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.h:99: std::set<CompilerDispatcherJob*> pending_background_jobs_; Why a set, why not a FIFO? (Ah, found out the answer to that question later, but leaving the comment here anyway.) Afaics this impl doesn't give a guarantee that any given task will be executed ever (it might be that we always pick some other task to run). Ofc if we really need it, then we run it... ----- So you chose set over a FIFO queue because you need to check whether a given job is there or not. Idk why not unordered_set though? https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.h:103: std::set<CompilerDispatcherJob*> running_background_jobs_; Ditto https://codereview.chromium.org/2606263002/diff/20001/test/unittests/compiler... File test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc (right): https://codereview.chromium.org/2606263002/diff/20001/test/unittests/compiler... test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:321: MockPlatform platform; You could make MockPlatform a member of CompilerDispatcherTest, so you wouldn't need to have it explicitly in each test... https://codereview.chromium.org/2606263002/diff/20001/test/unittests/compiler... test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:395: platform.RunBackgroundTasks(V8::GetCurrentPlatform()); For documenting what happens, you could assert the status of the job here. Afaics it's now compiled... You could also add another test for FinishNow:ing a task which is in the queue for background processing but not done yet. Looks like with this kind of tests you cannot easily test a situation where you're actually doing the background work when FinishNow is called... or can you mock that somehow too?
https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:284: CompilerDispatcherJob* job = nullptr; Can you add an assert (somewhere, maybe here) that the size of running_background_jobs_ doesn't exceed the number of available threads?
The CQ bit was checked by jochen@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...
ptal Had to make CompilerDispatcher have its own cancelable task manager (instead of relying on the isolate), so we can be sure that all background tasks are gone when we destroy the compiler dispatcher. Otherwise, there might be still inflight bg tasks, even though no jobs are available (however, they'll fail when they try to get the lock to check whether jobs are available) https://codereview.chromium.org/2606263002/diff/1/src/compiler-dispatcher/com... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2606263002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:344: } On 2017/01/03 at 10:32:37, vogelheim wrote: > On 2017/01/02 19:39:39, jochen wrote: > > On 2017/01/02 at 17:16:24, vogelheim wrote: > > > [opinion ahead; feel free to ignore:] > > > > > > > > > The can-idle or can-on-any-thread properties of the jobs don't really > > change... I wonder if it would be simpler to record those properties in the > > jobs, have a single mutex-locked queue of jobs, and then have the background and > > idle threads 'race' to pick available jobs of their respective type from that > > queue. > > > > > > (I think the current logic tries to handle this more intelligently by putting > > the different types of jobs into different queues/sets, but that 'distributes' > > the information across many data structures. I find that difficult to follow.) > > > > > > > > > Related (and mostly arguing against my own proposal): It would be nice to have > > some logic to make this --predictable, other than disabling the scheduler > > entirely. The explicit assignment to queues probably makes that simpler. I'm not > > really sure, though. > > > > the can-idle property can change, when the estimates get updated. > > > > I don't think we can ever make this predictable, as idle time is just not > > predictable.. > > Hmm. Not sure I'm willing to give this up yet. For --predictable, we could implement a behaviour that it will always execute exactly one idle task regardless of size for every DoIdleWork call, in strict enqueing order. That'd be predictable, provided DoIdleWork calls are predictable. > > Then d8 could have a platform implementation that has idle callbacks based on a similarly predictable scheme. > > That'd give us some test coverage for the scheduler & the idle tasks, while (mostly?) preserving predictability. in d8, we do get predictable idle work calls, however, in chrome (or other embedders) we don't. I'd rather keep --predictable to be really predictable, as this helps with debugging a lot https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:190: void CompilerDispatcher::EnsureNotOnBackground(CompilerDispatcherJob* job) { On 2017/01/03 at 10:42:41, marja wrote: > This function name is confusing. I had to read it several times to understand what it does... > > So, what it does: if a job is currently running on background, it waits until it's done. "BlockIfJobIsRunningOnBackground"? "WaitForJobIfRunningOnBackground"? done https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:201: DCHECK(pending_background_jobs_.find(job) == pending_background_jobs_.end()); On 2017/01/03 at 10:32:37, vogelheim wrote: > Also the same DCHECK(...), with running_background_jobs? > > The post-condition is that job is in neither of those queues, right? done https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:221: DCHECK(blocking == BlockingBehavior::kBlock); On 2017/01/03 at 10:32:37, vogelheim wrote: > DCHECK_EQ ? doesn't work with enum classes https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:269: if (FLAG_single_threaded) return; On 2017/01/03 at 10:32:37, vogelheim wrote: > Hmm. So if --single_threaded, ConsiderJobForBackgroundProcessing might put a job into pending_background_job_, and they'd just linger there until the main thread needs it and will FinishNow(job)? if the job is too big for idle time. Since the DoIdleWork() iterates over jobs_, it'll still process pending_background_jobs_ if it can https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:284: CompilerDispatcherJob* job = nullptr; On 2017/01/03 at 10:43:41, marja wrote: > Can you add an assert (somewhere, maybe here) that the size of running_background_jobs_ doesn't exceed the number of available threads? that assert will not necessarily hold: the embedder can spawn as many threads as it likes (available threads is just a hint) https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:307: ScheduleIdleTaskFromAnyThread(); On 2017/01/03 at 10:32:37, vogelheim wrote: > ScheduleIdleTaskFromAnyThread unconditionally schedules the idle thread. Why do we know that we need one here? Is it because any background-compile always needs at least one main-thread step to finish? correct. Added a comment https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:339: auto it = pending_background_jobs_.find(job->second.get()); On 2017/01/03 at 10:42:41, marja wrote: > This is kinda weird... here we erase the job from pending_background_jobs and then we potentially insert it again if it's too big to complete now. > > So once we're running out of idle time, we might go through the whole pending_background_jobs_ structure, taking each job out and putting it back in. That feels weird... > > How about taking the job out of pending_background_jobs_ only if we're going to work on it? we're always working on it in the sense that we invoke methods on the job. If I don't take the job out of the pending set, a background thread might race and start processing it, resulting in non-mutex protected accesses to the job https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:352: ConsiderJobForBackgroundProcessing(job->second.get()); On 2017/01/03 at 10:42:40, marja wrote: > This is somewhat surprising... so we only push those jobs which are too long to complete (now) for background processing. Why is that? the other cases are: - the job is finished (no need to push it) - we're doing a step with the job (and continue working on it) only if it's not yet finished, but we don't have enough time to work on it, we need to put it on the bg thread https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... File src/compiler-dispatcher/compiler-dispatcher.h (right): https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.h:99: std::set<CompilerDispatcherJob*> pending_background_jobs_; On 2017/01/03 at 10:42:41, marja wrote: > Why a set, why not a FIFO? (Ah, found out the answer to that question later, but leaving the comment here anyway.) > > Afaics this impl doesn't give a guarantee that any given task will be executed ever (it might be that we always pick some other task to run). Ofc if we really need it, then we run it... > > ----- > > So you chose set over a FIFO queue because you need to check whether a given job is there or not. Idk why not unordered_set though? changed to unordered_set https://codereview.chromium.org/2606263002/diff/20001/test/unittests/compiler... File test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc (right): https://codereview.chromium.org/2606263002/diff/20001/test/unittests/compiler... test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:53: i::FLAG_ignition = true; On 2017/01/03 at 10:32:37, vogelheim wrote: > Is this necessary? > > unittests/ uses gtest, which has a flag saver mechanism built in. Would this not cover this flag? I never heard of that? how does it work? I need to enable --ignition, otherwise compile jobs might end up using FCG which doesn't support background compilation https://codereview.chromium.org/2606263002/diff/20001/test/unittests/compiler... test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:97: idle_task_ = task; On 2017/01/03 at 10:32:37, vogelheim wrote: > ASSERT_TRUE(idle_task_ == nullptr) before this line? done https://codereview.chromium.org/2606263002/diff/20001/test/unittests/compiler... test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:138: for (auto&& task : tasks) { On 2017/01/03 at 10:32:37, vogelheim wrote: > C++ question: Why the auto&& ? > > (I take it this makes no appreciable difference for auto resolving to a Task*; but I guess it's a defensive programming thing that should work better in some cases?) no reason actually, using auto& now https://codereview.chromium.org/2606263002/diff/20001/test/unittests/compiler... test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:321: MockPlatform platform; On 2017/01/03 at 10:42:41, marja wrote: > You could make MockPlatform a member of CompilerDispatcherTest, so you wouldn't need to have it explicitly in each test... meh, unless you feel strongly about this https://codereview.chromium.org/2606263002/diff/20001/test/unittests/compiler... test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:358: platform.RunIdleTask(1000.0, 0.0); On 2017/01/03 at 10:32:37, vogelheim wrote: > [For my understanding:] What does this step accomplish? > > This test ultimately relies on the fact that a very expensive compile job will have exactly one background step? this step does the final step of compilation which needs to be on the main thread (copy bytecode to the heap and the shared function info) https://codereview.chromium.org/2606263002/diff/20001/test/unittests/compiler... test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:395: platform.RunBackgroundTasks(V8::GetCurrentPlatform()); On 2017/01/03 at 10:42:41, marja wrote: > For documenting what happens, you could assert the status of the job here. Afaics it's now compiled... > > You could also add another test for FinishNow:ing a task which is in the queue for background processing but not done yet. > > Looks like with this kind of tests you cannot easily test a situation where you're actually doing the background work when FinishNow is called... or can you mock that somehow too? RunBackgroundTasks() does not block, so this test creates a race between the background task and FinishNow()
still lgtm but I still have some of the nits I had :) https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:284: CompilerDispatcherJob* job = nullptr; On 2017/01/03 12:59:35, jochen wrote: > On 2017/01/03 at 10:43:41, marja wrote: > > Can you add an assert (somewhere, maybe here) that the size of > running_background_jobs_ doesn't exceed the number of available threads? > > that assert will not necessarily hold: the embedder can spawn as many threads as > it likes (available threads is just a hint) Hmm, I'm missing something... ScheduleMoreBackgroundTasksIfNeeded doesn't schedule more concurrent tasks than NumberOfAvailableBackgroundThreads(), and the only way to put things into running_background_jobs_ is to have a background task which does DoBackgroundWork. What's the other mechanism bypassing that? https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:339: auto it = pending_background_jobs_.find(job->second.get()); On 2017/01/03 12:59:36, jochen wrote: > On 2017/01/03 at 10:42:41, marja wrote: > > This is kinda weird... here we erase the job from pending_background_jobs and > then we potentially insert it again if it's too big to complete now. > > > > So once we're running out of idle time, we might go through the whole > pending_background_jobs_ structure, taking each job out and putting it back in. > That feels weird... > > > > How about taking the job out of pending_background_jobs_ only if we're going > to work on it? > > we're always working on it in the sense that we invoke methods on the job. If I > don't take the job out of the pending set, a background thread might race and > start processing it, resulting in non-mutex protected accesses to the job If the job is too big (or the amount of idle time too low), we don't work on it. Ofc the decision needs to be protected by the mutex, so, I meant something like this: grab mutex decide if we're going to work on the task if yes, remove it from the set release mutex do the work Or maybe even something like this: grab mutex find a task to work on (so, possibly loop over tasks here) remove it release mutex do the work https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:352: ConsiderJobForBackgroundProcessing(job->second.get()); On 2017/01/03 12:59:36, jochen wrote: > On 2017/01/03 at 10:42:40, marja wrote: > > This is somewhat surprising... so we only push those jobs which are too long > to complete (now) for background processing. Why is that? > > the other cases are: > > - the job is finished (no need to push it) > - we're doing a step with the job (and continue working on it) > > only if it's not yet finished, but we don't have enough time to work on it, we > need to put it on the bg thread But if there are a zillion of small tasks and several cores, don't we want the main thread and the background thread work on them simultaneously? (Guarantees / assumptions like that are not documented explicitly, so I'm continuously confused by the compiler dispatcher, since I assume it does something / gives a guarantee and then it does something else :) Could you add a top level comment into the header which would explain how this is supposed to work on a high level?) https://codereview.chromium.org/2606263002/diff/20001/test/unittests/compiler... File test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc (right): https://codereview.chromium.org/2606263002/diff/20001/test/unittests/compiler... test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:321: MockPlatform platform; On 2017/01/03 12:59:36, jochen wrote: > On 2017/01/03 at 10:42:41, marja wrote: > > You could make MockPlatform a member of CompilerDispatcherTest, so you > wouldn't need to have it explicitly in each test... > > meh, unless you feel strongly about this I don't :)
lgtm. More looks-goodness, more opinions. :) https://codereview.chromium.org/2606263002/diff/40001/src/compiler-dispatcher... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2606263002/diff/40001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:101: CompilerDispatcher* dispatcher); [Here, and CompilerDispatcher::IdleTask below]: Since the BackgroundTask (& IdleTask) always know the CompilerDispatcher*, you could give it an accessor CompilerDispatcher::cancelable_task_manager() and use that one, rather than requiring the client to pass it in. That way, you can't mismatch the task and the appropriate task manager. https://codereview.chromium.org/2606263002/diff/40001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:231: WaitForJobIfRunningOnBackground(kv.second.get()); If we want to abort the task (as in, we don't care whether they finish), and if we have cancel-able tasks, wouldn't we try to cancel those, instead of waiting for them? https://codereview.chromium.org/2606263002/diff/40001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:322: // deleted. I'm confused. (Might be me...) How can it be that *this is deleted here, but wasn't in line #318, where we merrily access *this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jochen@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...
https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:284: CompilerDispatcherJob* job = nullptr; On 2017/01/03 at 13:14:57, marja wrote: > On 2017/01/03 12:59:35, jochen wrote: > > On 2017/01/03 at 10:43:41, marja wrote: > > > Can you add an assert (somewhere, maybe here) that the size of > > running_background_jobs_ doesn't exceed the number of available threads? > > > > that assert will not necessarily hold: the embedder can spawn as many threads as > > it likes (available threads is just a hint) > > Hmm, I'm missing something... > > ScheduleMoreBackgroundTasksIfNeeded doesn't schedule more concurrent tasks than NumberOfAvailableBackgroundThreads(), and the only way to put things into running_background_jobs_ is to have a background task which does DoBackgroundWork. What's the other mechanism bypassing that? assume that each task counts down num_scheduled_background_tasks & inserts itself in running_background_jobs_ and then starves. The main thread will then continue to schedule new jobs that all could suffer the same faith. https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:339: auto it = pending_background_jobs_.find(job->second.get()); On 2017/01/03 at 13:14:57, marja wrote: > On 2017/01/03 12:59:36, jochen wrote: > > On 2017/01/03 at 10:42:41, marja wrote: > > > This is kinda weird... here we erase the job from pending_background_jobs and > > then we potentially insert it again if it's too big to complete now. > > > > > > So once we're running out of idle time, we might go through the whole > > pending_background_jobs_ structure, taking each job out and putting it back in. > > That feels weird... > > > > > > How about taking the job out of pending_background_jobs_ only if we're going > > to work on it? > > > > we're always working on it in the sense that we invoke methods on the job. If I > > don't take the job out of the pending set, a background thread might race and > > start processing it, resulting in non-mutex protected accesses to the job > > If the job is too big (or the amount of idle time too low), we don't work on it. > > Ofc the decision needs to be protected by the mutex, so, I meant something like this: > > grab mutex > decide if we're going to work on the task > if yes, remove it from the set > release mutex > do the work > > Or maybe even something like this: > grab mutex > find a task to work on (so, possibly loop over tasks here) > remove it > release mutex > do the work what about this? https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:352: ConsiderJobForBackgroundProcessing(job->second.get()); On 2017/01/03 at 13:14:57, marja wrote: > On 2017/01/03 12:59:36, jochen wrote: > > On 2017/01/03 at 10:42:40, marja wrote: > > > This is somewhat surprising... so we only push those jobs which are too long > > to complete (now) for background processing. Why is that? > > > > the other cases are: > > > > - the job is finished (no need to push it) > > - we're doing a step with the job (and continue working on it) > > > > only if it's not yet finished, but we don't have enough time to work on it, we > > need to put it on the bg thread > > But if there are a zillion of small tasks and several cores, don't we want the main thread and the background thread work on them simultaneously? > > (Guarantees / assumptions like that are not documented explicitly, so I'm continuously confused by the compiler dispatcher, since I assume it does something / gives a guarantee and then it does something else :) Could you add a top level comment into the header which would explain how this is supposed to work on a high level?) added a lengthy comment https://codereview.chromium.org/2606263002/diff/40001/src/compiler-dispatcher... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2606263002/diff/40001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:101: CompilerDispatcher* dispatcher); On 2017/01/03 at 13:23:02, vogelheim wrote: > [Here, and CompilerDispatcher::IdleTask below]: > > Since the BackgroundTask (& IdleTask) always know the CompilerDispatcher*, you could give it an accessor CompilerDispatcher::cancelable_task_manager() and use that one, rather than requiring the client to pass it in. That way, you can't mismatch the task and the appropriate task manager. that's true, but I like dependency injection https://codereview.chromium.org/2606263002/diff/40001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:231: WaitForJobIfRunningOnBackground(kv.second.get()); On 2017/01/03 at 13:23:02, vogelheim wrote: > If we want to abort the task (as in, we don't care whether they finish), and if we have cancel-able tasks, wouldn't we try to cancel those, instead of waiting for them? we still need to reset the job on the main thread, so the background task needs to schedule this once it's done https://codereview.chromium.org/2606263002/diff/40001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:322: // deleted. On 2017/01/03 at 13:23:02, vogelheim wrote: > I'm confused. (Might be me...) How can it be that *this is deleted here, but wasn't in line #318, where we merrily access *this? NotifyOne() might unblock the main thread which then proceeds to run the dtor
https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2606263002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:284: CompilerDispatcherJob* job = nullptr; On 2017/01/03 14:01:44, jochen wrote: > On 2017/01/03 at 13:14:57, marja wrote: > > On 2017/01/03 12:59:35, jochen wrote: > > > On 2017/01/03 at 10:43:41, marja wrote: > > > > Can you add an assert (somewhere, maybe here) that the size of > > > running_background_jobs_ doesn't exceed the number of available threads? > > > > > > that assert will not necessarily hold: the embedder can spawn as many > threads as > > > it likes (available threads is just a hint) > > > > Hmm, I'm missing something... > > > > ScheduleMoreBackgroundTasksIfNeeded doesn't schedule more concurrent tasks > than NumberOfAvailableBackgroundThreads(), and the only way to put things into > running_background_jobs_ is to have a background task which does > DoBackgroundWork. What's the other mechanism bypassing that? > > assume that each task counts down num_scheduled_background_tasks & inserts > itself in running_background_jobs_ and then starves. The main thread will then > continue to schedule new jobs that all could suffer the same faith. Ah I see It's slightly unintuitive though that num_scheduled_background_tasks_ is not the number of tasks in flight, since there can be many which are currently doing their work... oh well. The name says "scheduled" though so it's good. https://codereview.chromium.org/2606263002/diff/60001/src/compiler-dispatcher... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2606263002/diff/60001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:344: // Don't work on jobs that are being worked on by background tasks. Ok, this version looks better in the sense that it doesn't shuffle the data structures so much. Only the locking is a bit wonky now that we need to explicitly reset()... but as there are 2 cases where we need to reset, it's not trivial to rewrite that. https://codereview.chromium.org/2606263002/diff/60001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:372: break; Btw, why do we break here, shouldn't we continue instead? https://codereview.chromium.org/2606263002/diff/60001/src/compiler-dispatcher... File src/compiler-dispatcher/compiler-dispatcher.h (right): https://codereview.chromium.org/2606263002/diff/60001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.h:38: // finalization step that happens on the main thread, every task first has to Grammar nit: every task has to be advanced during idle time first https://codereview.chromium.org/2606263002/diff/60001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.h:54: // possible during idle time. If a can't be advanced, but job is suitable for Grammar nit: if a job can't be advanced, but is suitable... https://codereview.chromium.org/2606263002/diff/60001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.h:55: // background processing, it fires of background threads. Grammar nit: fires *off*
The CQ bit was checked by jochen@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...
grammar nits addressed... https://codereview.chromium.org/2606263002/diff/60001/src/compiler-dispatcher... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2606263002/diff/60001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:372: break; On 2017/01/03 at 14:22:45, marja wrote: > Btw, why do we break here, shouldn't we continue instead? good catch
The CQ bit was unchecked by jochen@chromium.org
The CQ bit was checked by jochen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vogelheim@chromium.org, marja@chromium.org Link to the patchset: https://codereview.chromium.org/2606263002/#ps80001 (title: "updates")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483454085493170,
"parent_rev": "72c370767226cf573d316655b1d3e3d3d699cc9b", "commit_rev":
"7a1b3a7bebbef88e72c9f7747e1930f10ee10c80"}
Message was sent while issue was closed.
Description was changed from ========== Use background tasks for the compiler dispatcher BUG=v8:5215 R=marja@chromium.org,vogelheim@chromium.org ========== to ========== Use background tasks for the compiler dispatcher BUG=v8:5215 R=marja@chromium.org,vogelheim@chromium.org Review-Url: https://codereview.chromium.org/2606263002 Cr-Commit-Position: refs/heads/master@{#42035} Committed: https://chromium.googlesource.com/v8/v8/+/7a1b3a7bebbef88e72c9f7747e1930f10ee... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/v8/v8/+/7a1b3a7bebbef88e72c9f7747e1930f10ee...
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2614433002/ by rmcilroy@chromium.org. The reason for reverting is: Causes IgnitionCompilerDispatcherTest.FinishNowWithBackgroundTask to fail. https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20noi18n%20-%2.... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
