Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2573493002:
Use idle time to make progress on scheduled compilation jobs (Closed)
|
Created:
4 years ago by jochen (gone - plz use gerrit) Modified:
4 years ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionUse idle time to make progress on scheduled compilation jobs
BUG=v8:5215
R=rmcilroy@chromium.org,marja@chromium.org,vogelheim@chromium.org
Review-Url: https://codereview.chromium.org/2573493002
Cr-Commit-Position: refs/heads/master@{#41767}
Committed: https://chromium.googlesource.com/v8/v8/+/692ba84f4fa30df106129d6f376448b4b421e892
Patch Set 1 #
Total comments: 22
Patch Set 2 : updates #
Total comments: 26
Patch Set 3 : updates #Patch Set 4 : updates #Patch Set 5 : add comment #
Total comments: 1
Patch Set 6 : Daniel's loop variant #
Total comments: 1
Patch Set 7 : updates #
Total comments: 7
Patch Set 8 : updates #Messages
Total messages: 57 (33 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...
Description was changed from ========== Use idle time to make progress on scheduled compilation jobs BUG=v8:5215 R=rmcilroy@chromium.org,marja@chromium.org ========== to ========== Use idle time to make progress on scheduled compilation jobs BUG=v8:5215 R=rmcilroy@chromium.org,marja@chromium.org,vogelheim@chromium.org ==========
jochen@chromium.org changed reviewers: + vogelheim@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks good, but I'm not sure I fully understand it. Some comments below. https://codereview.chromium.org/2573493002/diff/1/src/compiler-dispatcher/com... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2573493002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:75: // CancelableIdleTask implementation. I don't understand the comment. If the intention is to underline that this implements a super-class method, doesn't override already say that? https://codereview.chromium.org/2573493002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:204: it->second->EstimateRuntimeOfNextStepInMs() / Every one of these will look the same lock, right? Maybe lock it once outside the loop? [Probably won't be a big deal, though.] https://codereview.chromium.org/2573493002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:216: runtimes.pop(); If runtimes is a priority queue by expected runtime, wouldn't it suffice to exit the loop when the 1st 'too large' job is encountered? https://codereview.chromium.org/2573493002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:216: runtimes.pop(); [Not sure:] What happens when there is a job whose runtime estimate is very large? We'll re-consider it & drop it on every idle task, until the function is finally run (&compiled) by the main thread? https://codereview.chromium.org/2573493002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:235: job)); [Not sure:] It might make sense to store auto current = runtimes.top() JobMap::const_iterator job& = current.second; up there, in which case you wouldn't need to rebuild the pair here (would drop one mutex call). https://codereview.chromium.org/2573493002/diff/1/test/unittests/compiler-dis... File test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc (right): https://codereview.chromium.org/2573493002/diff/1/test/unittests/compiler-dis... test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:73: std::unique_ptr<MockPlatform> platform(new MockPlatform); [Here & below:] Why not just have stack allocated members? MockPlatform platform; CompilerDispatcher dispatcher(...);
https://codereview.chromium.org/2573493002/diff/1/src/compiler-dispatcher/com... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2573493002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:75: // CancelableIdleTask implementation. On 2016/12/12 at 17:51:32, vogelheim wrote: > I don't understand the comment. If the intention is to underline that this implements a super-class method, doesn't override already say that? it's a common pattern in chromium code as it has multiple inheritance, so you can easily see which methods come from which base class: https://cs.chromium.org/search/?q=//%5C+.*%5C+implementation%5C.$+file:h$ https://codereview.chromium.org/2573493002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:204: it->second->EstimateRuntimeOfNextStepInMs() / On 2016/12/12 at 17:51:32, vogelheim wrote: > Every one of these will look the same lock, right? Maybe lock it once outside the loop? > > [Probably won't be a big deal, though.] right, dunno how much of an issue that is? https://codereview.chromium.org/2573493002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:216: runtimes.pop(); On 2016/12/12 at 17:51:32, vogelheim wrote: > If runtimes is a priority queue by expected runtime, wouldn't it suffice to exit the loop when the 1st 'too large' job is encountered? So far, the biggest task comes first. I guess I could invert the ordering and let the smallest task be processed first. > [Not sure:] What happens when there is a job whose runtime estimate is very large? > > We'll re-consider it & drop it on every idle task, until the function is finally run (&compiled) by the main thread? either by the main thread, or a background thread. the thing is, once a background thread made progress on a task, we have to do the next step on the main thread again (and it's hopefully a small finalization step), so I can't permanently remove jobs from being considered for idle time compilation. maybe there should be a list of jobs that are handled by the background part (once it exists) https://codereview.chromium.org/2573493002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:235: job)); On 2016/12/12 at 17:51:32, vogelheim wrote: > [Not sure:] It might make sense to store > auto current = runtimes.top() > JobMap::const_iterator job& = current.second; > up there, in which case you wouldn't need to rebuild the pair here (would drop one mutex call). we still need the mutex, because after having made one step, we need to estimate the length for the next step here
https://codereview.chromium.org/2573493002/diff/1/src/compiler-dispatcher/com... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2573493002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:59: } Can we assert here that if the try catch has caught, the status is kFailed? https://codereview.chromium.org/2573493002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:187: idle_task_scheduled_ = false; Could you add assert(we're on the main thread) to the relevant functions of this class? https://codereview.chromium.org/2573493002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:190: // idle time and sort by how long they're likely to take. Hmm, it looks inefficient to iterate through all idle tasks every time we're idle for a moment - can we cache some of this information? https://codereview.chromium.org/2573493002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:213: deadline_in_seconds - platform_->MonotonicallyIncreasingTime()) { This loop is mildly confusing. Why not just: while (double idle_time_in_seconds = deadline_in_seconds - platform_->MonotonicallyIncreasingTime() > 0) { ... } https://codereview.chromium.org/2573493002/diff/1/test/unittests/compiler-dis... File test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc (right): https://codereview.chromium.org/2573493002/diff/1/test/unittests/compiler-dis... test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:167: Can you add the following tests Test 1: - Do something which creates 2 idle tasks - Do RunIdleTask(something small); - Assert that only one of the tasks was processed. Test 2: - Assert that your tasks get processed in the order you'd expect them to.
https://codereview.chromium.org/2573493002/diff/1/src/compiler-dispatcher/com... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2573493002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:209: for (double idle_time_in_seconds = What if coming up with the priority queue takes so long that we have no time left here? What should happen - should we always advance at least one task?
https://codereview.chromium.org/2573493002/diff/1/src/compiler-dispatcher/com... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2573493002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:75: // CancelableIdleTask implementation. On 2016/12/12 20:28:25, jochen wrote: > On 2016/12/12 at 17:51:32, vogelheim wrote: > > I don't understand the comment. If the intention is to underline that this > implements a super-class method, doesn't override already say that? > > it's a common pattern in chromium code as it has multiple inheritance, so you > can easily see which methods come from which base class: > https://cs.chromium.org/search/?q=//%5C+.*%5C+implementation%5C.$+file:h$ Acknowledged. https://codereview.chromium.org/2573493002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:216: runtimes.pop(); > > If runtimes is a priority queue by expected runtime, wouldn't it suffice to exit the loop when the 1st 'too large' job is encountered? > So far, the biggest task comes first. I guess I could invert the ordering and let the smallest task be processed first. Ah, I misread that and assumed we'd process tasks in ascending order. When sorted in descending order it makes sense. https://codereview.chromium.org/2573493002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:235: job)); On 2016/12/12 20:28:25, jochen wrote: > On 2016/12/12 at 17:51:32, vogelheim wrote: > > [Not sure:] It might make sense to store > > auto current = runtimes.top() > > JobMap::const_iterator job& = current.second; > > up there, in which case you wouldn't need to rebuild the pair here (would drop > one mutex call). > > we still need the mutex, because after having made one step, we need to estimate > the length for the next step here Ah. For my understanding: Why would the time estimate change, vs the one the job had assigned in line #220 ? From reading the code, it seems to compute the average progress rate of the various steps. So technically it would be a different value, but.. not by much, right?
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...
next attempt, ptal https://codereview.chromium.org/2573493002/diff/1/src/compiler-dispatcher/com... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2573493002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:59: } On 2016/12/13 at 09:09:01, marja wrote: > Can we assert here that if the try catch has caught, the status is kFailed? done https://codereview.chromium.org/2573493002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher.cc:235: job)); On 2016/12/13 at 09:40:33, vogelheim wrote: > On 2016/12/12 20:28:25, jochen wrote: > > On 2016/12/12 at 17:51:32, vogelheim wrote: > > > [Not sure:] It might make sense to store > > > auto current = runtimes.top() > > > JobMap::const_iterator job& = current.second; > > > up there, in which case you wouldn't need to rebuild the pair here (would drop > > one mutex call). > > > > we still need the mutex, because after having made one step, we need to estimate > > the length for the next step here > > > Ah. > > For my understanding: Why would the time estimate change, vs the one the job had assigned in line #220 ? > > From reading the code, it seems to compute the average progress rate of the various steps. So technically it would be a different value, but.. not by much, right? in line 222 the job does one step, and i gets reinserted with the estimate for the next step https://codereview.chromium.org/2573493002/diff/1/test/unittests/compiler-dis... File test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc (right): https://codereview.chromium.org/2573493002/diff/1/test/unittests/compiler-dis... test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:73: std::unique_ptr<MockPlatform> platform(new MockPlatform); On 2016/12/12 at 17:51:32, vogelheim wrote: > [Here & below:] Why not just have stack allocated members? > > MockPlatform platform; > CompilerDispatcher dispatcher(...); done
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm (a few nitpicks, however) https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... File src/compiler-dispatcher/compiler-dispatcher-tracer.cc (right): https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher-tracer.cc:158: double sum = buffer.Sum([](double a, double b) { return a + b; }, 0.0); It seems like Average/Estimate will be called at least as often as the Estimate* functions, so it would make sense to maintain a running sum when adding to the buffer. [Not sure if it makes sense. And since it's a bounded buffer we're already O(1). :) ] https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher-tracer.cc:164: if (buffer.Count() == 0) return kEstimatedRuntimeWithoutData; Why do you sometimes default to 0.0 and sometimes to kEstimatedRuntimeWithoutData? [I'm not seeing any problem; but I don't understand the thinking.] https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:57: DCHECK(job->status() == CompileJobStatus::kFailed); nitpick: DCHECK_EQ https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:68: // Theoretically, we get 50ms of idle time max, however, it's unlikely that super nitpick: Remove some of the commas, like: Theoretically we get 50ms of idle time max, however it's unlikely [...] https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:253: if (work_left) ScheduleIdleTaskIfNeeded(); At this point, wouldn't (work_left == !job_set.empty()) ? If so, you don't need the variable at all. If not... then I didn't get it. :) https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:253: if (work_left) ScheduleIdleTaskIfNeeded(); This would potentially be called twice, since DoIdleWorkForJobSet is called for two different job sets. Maybe pull the check into DoIdleWork? https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:265: } style nitpick; feel free to ignore... The if-else effectively implements an or-operator, so the following might be more readable: bool can_go_on_background_thread = (job->status() == CompileJobStatus::kReadyToParse && job->can_parse_on_background_thread()) || (job->status() == CompileJobStatus::kReadyToCompile && job->can_compile_on_background_thread()); https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... File src/compiler-dispatcher/compiler-dispatcher.h (right): https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.h:61: std::unique_ptr<CompilerDispatcherJob>> nitpick: Maybe typedef std::unordered_set<JobMap::const_iterator, JobIteratorHash> to something? You're using it a number of times in this header. https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.h:67: std::hash<int> hasher; Why std::hash<int>, and not v8::base::hash<int>? (Or are those the same? It doesn't look like they are...) https://codereview.chromium.org/2573493002/diff/20001/test/unittests/compiler... File test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc (right): https://codereview.chromium.org/2573493002/diff/20001/test/unittests/compiler... test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:6: #include "include/v8-platform.h" "includ..." < "src/..." (If you want to include the 'matching' header first, then add a newline to designate a new header 'block'. And if clang-format disagrees, do whatever...)
https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... File src/compiler-dispatcher/compiler-dispatcher-tracer.cc (right): https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher-tracer.cc:164: if (buffer.Count() == 0) return kEstimatedRuntimeWithoutData; On 2016/12/15 at 15:24:11, vogelheim wrote: > Why do you sometimes default to 0.0 and sometimes to kEstimatedRuntimeWithoutData? > > [I'm not seeing any problem; but I don't understand the thinking.] I take the 1.0 estimate for parse & compile, and zero for all the finalize/prepare steps that are hopefully super fast. https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:57: DCHECK(job->status() == CompileJobStatus::kFailed); On 2016/12/15 at 15:24:12, vogelheim wrote: > nitpick: DCHECK_EQ that would require implementation operator<< for the enum class :-/ https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:68: // Theoretically, we get 50ms of idle time max, however, it's unlikely that On 2016/12/15 at 15:24:11, vogelheim wrote: > super nitpick: Remove some of the commas, like: > Theoretically we get 50ms of idle time max, however it's unlikely [...] done https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:253: if (work_left) ScheduleIdleTaskIfNeeded(); On 2016/12/15 at 15:24:12, vogelheim wrote: >This would potentially be called twice, since DoIdleWorkForJobSet is called for two different job sets. Maybe pull the check into DoIdleWork? I found it more readable to put it here, instead of having a magic bool return value > At this point, wouldn't (work_left == !job_set.empty()) ? > > If so, you don't need the variable at all. If not... then I didn't get it. :) job_set could be full of jobs that are estimated to take more than the max available idle time https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:265: } On 2016/12/15 at 15:24:11, vogelheim wrote: > style nitpick; feel free to ignore... The if-else effectively implements an or-operator, so the following might be more readable: > > bool can_go_on_background_thread = > (job->status() == CompileJobStatus::kReadyToParse && > job->can_parse_on_background_thread()) || > (job->status() == CompileJobStatus::kReadyToCompile && > job->can_compile_on_background_thread()); done https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... File src/compiler-dispatcher/compiler-dispatcher.h (right): https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.h:61: std::unique_ptr<CompilerDispatcherJob>> On 2016/12/15 at 15:24:12, vogelheim wrote: > nitpick: Maybe typedef std::unordered_set<JobMap::const_iterator, JobIteratorHash> to something? You're using it a number of times in this header. done https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.h:67: std::hash<int> hasher; On 2016/12/15 at 15:24:12, vogelheim wrote: > Why std::hash<int>, and not v8::base::hash<int>? > > (Or are those the same? It doesn't look like they are...) no reason, fixed https://codereview.chromium.org/2573493002/diff/20001/test/unittests/compiler... File test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc (right): https://codereview.chromium.org/2573493002/diff/20001/test/unittests/compiler... test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:6: #include "include/v8-platform.h" On 2016/12/15 at 15:24:12, vogelheim wrote: > "includ..." < "src/..." > > (If you want to include the 'matching' header first, then add a newline to designate a new header 'block'. And if clang-format disagrees, do whatever...) done
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/2573493002/diff/20001/src/compiler-dispatcher... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:208: void CompilerDispatcher::DoIdleWorkForJobSet( Can you add some high-level comments to CompilerDispatcher about what it does and how? E.g., that it goes through the job set in an undefined order and how all the different job queues and sets interact with each other. https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:224: it = jobs_for_main_thread_.erase(it); How is this supposed to work? it is an iterator for job_set and now you jobs_for_main_thread_.erase with it, and moreover, save the returned iterator back. That shouldn't do anything meaningful afaics. Why doesn't any of your tests fail? :) (Hmm, job_set can be the same as jobs_for_main_thread_ but it can also be jobs_for_backround_thread_.) https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:237: ClassifyJob(job); The interaction between the queues and the job set is unclear after just skimming through the code. Does the job end up in the job set in multiple times even though its next step is too big to complete during idle time? Hmm, I read the code more, and looks indeed like it. Why does the job need to be in the sets if the only way for it to get processed is that the function it represents needs to be executed? Hmm2, looks like it's possible to have big functions that are too big to process as idle tasks but never called, and we'll just happily keep the tasks (and whatever memory they hang on to) alive. Is that intentional? https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:237: ClassifyJob(job); As job_set is a pointer to one of the underlying unordered_sets, and we modify them in ClassifyJob, it's slightly non-trivial to see that the modifications are OK and don't invalidate any iterators. I think they are now (I googled), it's just slightly error prone... if somebody changes the underlying data structure. Can you add a comment to where jobs_on_main_thread_ and so are declared about that? (That they need to be data structures where modifications don't invalidate iterators.) https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:245: jobs_.erase(job); There's jobs_ and job_set, could you name them somehow in a more illustrative manner? https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:256: void CompilerDispatcher::ClassifyJob(JobMap::const_iterator it) { This function name is misleading, it doesn't just classify the job but it also puts it into the right queue.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
here's a simpler version https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:224: it = jobs_for_main_thread_.erase(it); On 2016/12/15 at 15:56:43, marja wrote: > How is this supposed to work? it is an iterator for job_set and now you jobs_for_main_thread_.erase with it, and moreover, save the returned iterator back. That shouldn't do anything meaningful afaics. Why doesn't any of your tests fail? :) > > (Hmm, job_set can be the same as jobs_for_main_thread_ but it can also be jobs_for_backround_thread_.) erase() in C++11 returns the next iterator, it's short for auto old = it++; set.erase(old); https://codereview.chromium.org/2573493002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:237: ClassifyJob(job); On 2016/12/15 at 15:56:43, marja wrote: > The interaction between the queues and the job set is unclear after just skimming through the code. Does the job end up in the job set in multiple times even though its next step is too big to complete during idle time? > > Hmm, I read the code more, and looks indeed like it. Why does the job need to be in the sets if the only way for it to get processed is that the function it represents needs to be executed? > > Hmm2, looks like it's possible to have big functions that are too big to process as idle tasks but never called, and we'll just happily keep the tasks (and whatever memory they hang on to) alive. Is that intentional? once we also have a background scheduler, they'll make progress there
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2573493002/diff/80001/src/compiler-dispatcher... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2573493002/diff/80001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher.cc:231: } I like this version much better. --------------------------------- I'm still bothered by the loop-in-loop and the many loop exits, and toyed around a bit to find something where we could just have a single loop, that loops over jobs & time. This is admittedly entirely a matter of taste, so please feel free to ignore anything below. // Iterate over available jobs & remaining time. // For each job, decide whether to 1, skip it (if not enough time), 2, erase it (if finished), or 3, let it do some work. size_t hopeless_jobs = 0; double idle_time_in_seconds = deadline_in_seconds - platform_->MonotonicallyIncreasingTime(); for (auto job = jobs_.begin(); job != jobs.end() && idle_time_in_seconds > 0.0; idle_time_in_seconds = deadline_in_seconds - platform_->MonotonicallyIncreasingTime()) { double estimate_in_ms = ... if (estimate_in_ms <= kMaxIdleTimeToExcpectInMs) { // Skip: Job is too large for idle time. ++ job; ++ hopeless_jobs; } else if (idle_time_in_seconds < estimate_...) { // Skip: Not enough time left in this idle call. ++ job; } else if (IsFinished(job->second.get())) { // Erase: Job has already finished. job->second... job = jobs_erase(job); } else { // Make progress. Then revisit current job. DoNextStepOnMainThread(....) } } if (jobs_.size() > hopeless_jobs) ScheduleIdleTaskIfNeeded();
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...
implemented (almost) your version, wdyt?
On 2016/12/16 11:51:52, jochen wrote: > implemented (almost) your version, wdyt? Thank you, lgtm. The only, final nitpick I have is that there should be some comment that the else-branch purposely keeps the same job instance (maybe merged into or instead of the loop comment). I was initially confused about that aspect, and it's correct, but it _looks_ wrong because that's the only branch that doesn't modify the loop variable 'job'
https://codereview.chromium.org/2573493002/diff/100001/src/compiler-dispatche... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2573493002/diff/100001/src/compiler-dispatche... src/compiler-dispatcher/compiler-dispatcher.cc:223: DoNextStepOnMainThread(isolate_, job->second.get(), One (possibly unintended?) property of this code is that once we advance a job one step, if there's still some other job we can do, we advance that other job, instead of advancing the previous one again. Shouldn't we instead try to advance the current job as much as possible, so that it could potentially finish and release the memory it keeps alive?
haha, that's what the code does; I read it wrong. :) lgtm
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/2573493002/#ps120001 (title: "updates")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I like it, a nit and some questions, but LGTM https://codereview.chromium.org/2573493002/diff/120001/src/compiler-dispatche... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2573493002/diff/120001/src/compiler-dispatche... src/compiler-dispatcher/compiler-dispatcher.cc:20: enum class ExceptionHandling { kSwallow, kKeep }; nit - Keep->Throw ? https://codereview.chromium.org/2573493002/diff/120001/src/compiler-dispatche... src/compiler-dispatcher/compiler-dispatcher.cc:218: } else if (IsFinished(job->second.get())) { Question - do we always estimate that finishing a job takes zero time? Just wondering whether finishing jobs shouldn't care about idle time remaining? https://codereview.chromium.org/2573493002/diff/120001/src/compiler-dispatche... src/compiler-dispatcher/compiler-dispatcher.cc:226: ExceptionHandling::kSwallow); Do we loose the exception here or rethrow when finalising?
The CQ bit was unchecked by jochen@chromium.org
https://codereview.chromium.org/2573493002/diff/120001/src/compiler-dispatche... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2573493002/diff/120001/src/compiler-dispatche... src/compiler-dispatcher/compiler-dispatcher.cc:218: } else if (IsFinished(job->second.get())) { On 2016/12/16 at 12:16:07, rmcilroy wrote: > Question - do we always estimate that finishing a job takes zero time? Just wondering whether finishing jobs shouldn't care about idle time remaining? you mean the "ResetOnMainThread" operation (IsFinished() just checks the status). That just frees the memory held by the job. Of course that can take arbitrarily long if the system is swapping, but then you got bigger problems... https://codereview.chromium.org/2573493002/diff/120001/src/compiler-dispatche... src/compiler-dispatcher/compiler-dispatcher.cc:226: ExceptionHandling::kSwallow); On 2016/12/16 at 12:16:07, rmcilroy wrote: > Do we loose the exception here or rethrow when finalising? we lose it. DoNextStepOnMainThread also finalizes. That means that the SFI still has the CompileLazy stub, and once we execute it, we compile on the main thread and throw again.
The CQ bit was checked by jochen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org, vogelheim@chromium.org, marja@chromium.org Link to the patchset: https://codereview.chromium.org/2573493002/#ps140001 (title: "updates")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2573493002/diff/120001/src/compiler-dispatche... File src/compiler-dispatcher/compiler-dispatcher.cc (right): https://codereview.chromium.org/2573493002/diff/120001/src/compiler-dispatche... src/compiler-dispatcher/compiler-dispatcher.cc:218: } else if (IsFinished(job->second.get())) { On 2016/12/16 12:22:50, jochen wrote: > On 2016/12/16 at 12:16:07, rmcilroy wrote: > > Question - do we always estimate that finishing a job takes zero time? Just > wondering whether finishing jobs shouldn't care about idle time remaining? > > you mean the "ResetOnMainThread" operation (IsFinished() just checks the > status). That just frees the memory held by the job. Of course that can take > arbitrarily long if the system is swapping, but then you got bigger problems... i was more worried about us estimaring a time and never getting to finalize the job. i see now you always estinate 0.0 for these phases so this is good (hard to jump about CL reviewing on a phone....) . https://codereview.chromium.org/2573493002/diff/120001/src/compiler-dispatche... src/compiler-dispatcher/compiler-dispatcher.cc:226: ExceptionHandling::kSwallow); On 2016/12/16 12:22:50, jochen wrote: > On 2016/12/16 at 12:16:07, rmcilroy wrote: > > Do we loose the exception here or rethrow when finalising? > > we lose it. DoNextStepOnMainThread also finalizes. That means that the SFI still > has the CompileLazy stub, and once we execute it, we compile on the main thread > and throw again. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/19580) v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)
The CQ bit was checked by jochen@chromium.org
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": 140001, "attempt_start_ts": 1481897939416380,
"parent_rev": "c0d6939836f5eaf7580715ed58cab6930f3b5692", "commit_rev":
"692ba84f4fa30df106129d6f376448b4b421e892"}
Message was sent while issue was closed.
Description was changed from ========== Use idle time to make progress on scheduled compilation jobs BUG=v8:5215 R=rmcilroy@chromium.org,marja@chromium.org,vogelheim@chromium.org ========== to ========== Use idle time to make progress on scheduled compilation jobs BUG=v8:5215 R=rmcilroy@chromium.org,marja@chromium.org,vogelheim@chromium.org Review-Url: https://codereview.chromium.org/2573493002 Cr-Commit-Position: refs/heads/master@{#41767} Committed: https://chromium.googlesource.com/v8/v8/+/692ba84f4fa30df106129d6f376448b4b42... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/v8/v8/+/692ba84f4fa30df106129d6f376448b4b42... |
