Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
637983003:
Add support to base::Timer for custom task runners. (Closed)
|
Created:
6 years, 2 months ago by petrcermak Modified:
6 years, 1 month ago Reviewers:
Avi (use Gerrit), Sami, Lei Zhang, picksi1, danakj, rmcilroy, Mark Seaborn, alex clarke (OOO till 29th) CC:
chromium-reviews, erikwright+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd support to base::Timer for custom task runners.
This patch adds a public method
SetTaskRunner(scoped_refptr<SingleThreadTaskRunner>) to base::Timer
which changes the task runner used by the timer
(ThreadTaskRunnerHandler::Get() by default).
This change will enable scheduling timers on the queues of the
Blink scheduler (see https://codereview.chromium.org/637303003/).
Most importantly, we plan to apply this to the shared timer in
blink::Platform.
BUG=
Committed: https://crrev.com/7652da6d5cbd7e5b82b340e638ebe80aafeacf6e
Cr-Commit-Position: refs/heads/master@{#302947}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Update for review feedback #
Total comments: 19
Patch Set 3 : Update for review feedback #
Total comments: 10
Patch Set 4 : Update for review feedback #
Total comments: 8
Patch Set 5 : Final update for review feedback #Patch Set 6 : Clobber to fix missing NaCl gyp dependencies #Patch Set 7 : Rebased #Patch Set 8 : Final rebase #
Messages
Total messages: 35 (9 generated)
petrcermak@chromium.org changed reviewers: + alexclarke@chromium.org, picksi@chromium.org, rmcilroy@chromium.org, skyostil@chromium.org
Thanks Petr, looks great! https://codereview.chromium.org/637983003/diff/1/base/timer/timer.h File base/timer/timer.h (right): https://codereview.chromium.org/637983003/diff/1/base/timer/timer.h#newcode91 base/timer/timer.h:91: // Set the task runner on which the task should be scheduled. Please mention that this can only be changed before any tasks have been scheduled. https://codereview.chromium.org/637983003/diff/1/base/timer/timer.h#newcode153 base/timer/timer.h:153: scoped_refptr<SingleThreadTaskRunner> task_runner_; Could we initialize this to be ThreadTaskRunnerHandle::Get() in the constructor so we can avoid the dynamic resolving? They're the same types.
Oh, and could you expand on the motivation in the commit message? Right now it tells the what but not the why.
Thanks for the comments. I also updated the commit message as suggested. https://codereview.chromium.org/637983003/diff/1/base/timer/timer.h File base/timer/timer.h (right): https://codereview.chromium.org/637983003/diff/1/base/timer/timer.h#newcode91 base/timer/timer.h:91: // Set the task runner on which the task should be scheduled. On 2014/10/13 17:00:11, Sami wrote: > Please mention that this can only be changed before any tasks have been > scheduled. Done. https://codereview.chromium.org/637983003/diff/1/base/timer/timer.h#newcode153 base/timer/timer.h:153: scoped_refptr<SingleThreadTaskRunner> task_runner_; On 2014/10/13 17:00:10, Sami wrote: > Could we initialize this to be ThreadTaskRunnerHandle::Get() in the constructor > so we can avoid the dynamic resolving? They're the same types. As discussed, this cannot be done in the constructor because the task runner might not be constructed yet (see the TimerTest.MessageLoopShutdown unit test). Instead, we initialize it upon the first GetTaskRunner call.
Thanks, lgtm. Let's find an owner to have a look.
petrcermak@chromium.org changed reviewers: + danakj@chromium.org
danakj@chromium.org: Hi, could you please review my patch for base::Timer?
danakj@chromium.org changed reviewers: + avi@chromium.org
+avi As the author of this, can you please have a look at this for me? https://codereview.chromium.org/637983003/diff/110001/base/timer/timer.cc File base/timer/timer.cc (right): https://codereview.chromium.org/637983003/diff/110001/base/timer/timer.cc#new... base/timer/timer.cc:97: DCHECK(scheduled_task_ == NULL && thread_id_ == 0); these are 2 DCHECKs, split them up (or isn't checking thread_id enough?) also, DCHECK_EQ https://codereview.chromium.org/637983003/diff/110001/base/timer/timer.cc#new... base/timer/timer.cc:98: task_runner_ = task_runner; task_runner has a reference already, swap it into place so you don't add another reference here just to drop it https://codereview.chromium.org/637983003/diff/110001/base/timer/timer.cc#new... base/timer/timer.cc:171: task_runner_ = ThreadTaskRunnerHandle::Get(); This will make Timer hold a ref on the current task runner when it did not do so before. Let's not do that. Instead just grab the current one in PostNewScheduledTask when task_runner_ is null and use that instead of task_runner_. https://codereview.chromium.org/637983003/diff/110001/base/timer/timer_unitte... File base/timer/timer_unittest.cc (right): https://codereview.chromium.org/637983003/diff/110001/base/timer/timer_unitte... base/timer/timer_unittest.cc:164: f1.Start(); I think this test could be a bit more simple and also comprehensive. I'd picture something like f1.Start(); current()->Run(); // The timer task didn't run yet, it was not posted to the current loop. EXPECT_FALSE(..) task_runner->Run(); // The timer task ran now. EXPECT_TRUE(..) I'm not sure what the f2 and f3 in this test are testing? Are you checking for ordering? I don't see anything about ordering in this CL?
On 2014/10/16 17:15:00, danakj wrote: > +avi As the author of this, can you please have a look at this for me? Side note: I'm so not the author of this at all. I moved timer.h from base/ into base/timer/ so that's why my name is all over it.
Where is the bug #? https://codereview.chromium.org/637983003/diff/110001/base/timer/timer.cc File base/timer/timer.cc (right): https://codereview.chromium.org/637983003/diff/110001/base/timer/timer.cc#new... base/timer/timer.cc:97: DCHECK(scheduled_task_ == NULL && thread_id_ == 0); On 2014/10/16 17:14:59, danakj wrote: > these are 2 DCHECKs, split them up (or isn't checking thread_id enough?) > > also, DCHECK_EQ Yep. Two DCHECK_EQs, and nullptr. https://codereview.chromium.org/637983003/diff/110001/base/timer/timer.cc#new... base/timer/timer.cc:170: if (task_runner_.get() == NULL) { nullptr? https://codereview.chromium.org/637983003/diff/110001/base/timer/timer.h File base/timer/timer.h (right): https://codereview.chromium.org/637983003/diff/110001/base/timer/timer.h#newc... base/timer/timer.h:131: // corresponding task_runner_ field is NULL, the task runner for the current Here you say if the field is "NULL", but below you add a comment saying "If null". Pick one. https://codereview.chromium.org/637983003/diff/110001/base/timer/timer_unitte... File base/timer/timer_unittest.cc (right): https://codereview.chromium.org/637983003/diff/110001/base/timer/timer_unitte... base/timer/timer_unittest.cc:369: TEST(TimerTest, RunTest_OneShotTimer_CustomTaskRunner) { Drop "RunTest_" from the test name. https://codereview.chromium.org/637983003/diff/110001/base/timer/timer_unitte... base/timer/timer_unittest.cc:370: for (int i = 0; i < kNumTestingMessageLoops; i++) { Completely optional, but we have for ( : ) now if you want to use it here and elsewhere.
Patchset #3 (id:200001) has been deleted
Thanks for your comments. PTAL. https://codereview.chromium.org/637983003/diff/110001/base/timer/timer.cc File base/timer/timer.cc (right): https://codereview.chromium.org/637983003/diff/110001/base/timer/timer.cc#new... base/timer/timer.cc:97: DCHECK(scheduled_task_ == NULL && thread_id_ == 0); On 2014/10/16 18:02:55, Avi wrote: > On 2014/10/16 17:14:59, danakj wrote: > > these are 2 DCHECKs, split them up (or isn't checking thread_id enough?) > > > > also, DCHECK_EQ > > Yep. Two DCHECK_EQs, and nullptr. Done. I kept both checks since thread_id is set only after the task is posted. https://codereview.chromium.org/637983003/diff/110001/base/timer/timer.cc#new... base/timer/timer.cc:98: task_runner_ = task_runner; On 2014/10/16 17:14:59, danakj wrote: > task_runner has a reference already, swap it into place so you don't add another > reference here just to drop it Done. https://codereview.chromium.org/637983003/diff/110001/base/timer/timer.cc#new... base/timer/timer.cc:170: if (task_runner_.get() == NULL) { On 2014/10/16 18:02:55, Avi wrote: > nullptr? Done. https://codereview.chromium.org/637983003/diff/110001/base/timer/timer.cc#new... base/timer/timer.cc:171: task_runner_ = ThreadTaskRunnerHandle::Get(); On 2014/10/16 17:14:59, danakj wrote: > This will make Timer hold a ref on the current task runner when it did not do so > before. Let's not do that. > > Instead just grab the current one in PostNewScheduledTask when task_runner_ is > null and use that instead of task_runner_. Done. I did this initially but I was told to store the ref on the current task runner. https://codereview.chromium.org/637983003/diff/110001/base/timer/timer.h File base/timer/timer.h (right): https://codereview.chromium.org/637983003/diff/110001/base/timer/timer.h#newc... base/timer/timer.h:131: // corresponding task_runner_ field is NULL, the task runner for the current On 2014/10/16 18:02:55, Avi wrote: > Here you say if the field is "NULL", but below you add a comment saying "If > null". Pick one. Done (use "is NULL"). https://codereview.chromium.org/637983003/diff/110001/base/timer/timer_unitte... File base/timer/timer_unittest.cc (right): https://codereview.chromium.org/637983003/diff/110001/base/timer/timer_unitte... base/timer/timer_unittest.cc:164: f1.Start(); On 2014/10/16 17:14:59, danakj wrote: > I think this test could be a bit more simple and also comprehensive. I'd picture > something like > > f1.Start(); > current()->Run(); > // The timer task didn't run yet, it was not posted to the current loop. > EXPECT_FALSE(..) > > task_runner->Run(); > // The timer task ran now. > EXPECT_TRUE(..) > > I'm not sure what the f2 and f3 in this test are testing? Are you checking for > ordering? I don't see anything about ordering in this CL? My point was to check that the main loop and the task runner run their timers independently (i.e. that running the main loop doesn't trigger timers on other task runners and vice versa). If you think that this is unnecessary, I will change it as you suggested. https://codereview.chromium.org/637983003/diff/110001/base/timer/timer_unitte... base/timer/timer_unittest.cc:369: TEST(TimerTest, RunTest_OneShotTimer_CustomTaskRunner) { On 2014/10/16 18:02:55, Avi wrote: > Drop "RunTest_" from the test name. Done. https://codereview.chromium.org/637983003/diff/110001/base/timer/timer_unitte... base/timer/timer_unittest.cc:370: for (int i = 0; i < kNumTestingMessageLoops; i++) { On 2014/10/16 18:02:55, Avi wrote: > Completely optional, but we have for ( : ) now if you want to use it here and > elsewhere. Acknowledged. I kept the original for loop for consistency within the file.
https://codereview.chromium.org/637983003/diff/110001/base/timer/timer_unitte... File base/timer/timer_unittest.cc (right): https://codereview.chromium.org/637983003/diff/110001/base/timer/timer_unitte... base/timer/timer_unittest.cc:164: f1.Start(); On 2014/10/17 15:43:15, petrcermak wrote: > On 2014/10/16 17:14:59, danakj wrote: > > I think this test could be a bit more simple and also comprehensive. I'd > picture > > something like > > > > f1.Start(); > > current()->Run(); > > // The timer task didn't run yet, it was not posted to the current loop. > > EXPECT_FALSE(..) > > > > task_runner->Run(); > > // The timer task ran now. > > EXPECT_TRUE(..) > > > > I'm not sure what the f2 and f3 in this test are testing? Are you checking for > > ordering? I don't see anything about ordering in this CL? > > My point was to check that the main loop and the task runner run their timers > independently (i.e. that running the main loop doesn't trigger timers on other > task runners and vice versa). If you think that this is unnecessary, I will > change it as you suggested. I think you just need to test where the task gets posted to, not that the two message loops act independently. That's a test for the MessageLoop class. https://codereview.chromium.org/637983003/diff/220001/base/timer/timer.cc File base/timer/timer.cc (right): https://codereview.chromium.org/637983003/diff/220001/base/timer/timer.cc#new... base/timer/timer.cc:97: DCHECK(scheduled_task_ == nullptr); use DCHECK_EQ i still don't understand what this is adding over the thread_id_ check. are you worrying about this being called in the same stack frame from PostNewScheduledTask? I don't think that's possible since the task is posted, and it would get a new stack frame, and the thread_id_ would be set by then? https://codereview.chromium.org/637983003/diff/220001/base/timer/timer.cc#new... base/timer/timer.cc:98: DCHECK(thread_id_ == 0); use DCHECK_EQ https://codereview.chromium.org/637983003/diff/220001/base/timer/timer.cc#new... base/timer/timer.cc:171: return task_runner_.get() == nullptr ? == nullptr is redundant here return task_runner_.get() ? task_runner_ : RunnerHandle::Get() ? https://codereview.chromium.org/637983003/diff/220001/base/timer/timer.h File base/timer/timer.h (right): https://codereview.chromium.org/637983003/diff/220001/base/timer/timer.h#newc... base/timer/timer.h:131: // corresponding task_runner_ field is NULL, the task runner for the current NULL -> empty or null? https://codereview.chromium.org/637983003/diff/220001/base/timer/timer.h#newc... base/timer/timer.h:152: // The task runner on which the task should be scheduled. If it is NULL, the NULL -> empty or null?
Thanks for your comments. PTAL. https://codereview.chromium.org/637983003/diff/110001/base/timer/timer_unitte... File base/timer/timer_unittest.cc (right): https://codereview.chromium.org/637983003/diff/110001/base/timer/timer_unitte... base/timer/timer_unittest.cc:164: f1.Start(); On 2014/10/18 18:59:59, danakj wrote: > On 2014/10/17 15:43:15, petrcermak wrote: > > On 2014/10/16 17:14:59, danakj wrote: > > > I think this test could be a bit more simple and also comprehensive. I'd > > picture > > > something like > > > > > > f1.Start(); > > > current()->Run(); > > > // The timer task didn't run yet, it was not posted to the current loop. > > > EXPECT_FALSE(..) > > > > > > task_runner->Run(); > > > // The timer task ran now. > > > EXPECT_TRUE(..) > > > > > > I'm not sure what the f2 and f3 in this test are testing? Are you checking > for > > > ordering? I don't see anything about ordering in this CL? > > > > My point was to check that the main loop and the task runner run their timers > > independently (i.e. that running the main loop doesn't trigger timers on other > > task runners and vice versa). If you think that this is unnecessary, I will > > change it as you suggested. > > I think you just need to test where the task gets posted to, not that the two > message loops act independently. That's a test for the MessageLoop class. Done. https://codereview.chromium.org/637983003/diff/220001/base/timer/timer.cc File base/timer/timer.cc (right): https://codereview.chromium.org/637983003/diff/220001/base/timer/timer.cc#new... base/timer/timer.cc:97: DCHECK(scheduled_task_ == nullptr); On 2014/10/18 19:00:00, danakj wrote: > use DCHECK_EQ > > i still don't understand what this is adding over the thread_id_ check. are you > worrying about this being called in the same stack frame from > PostNewScheduledTask? I don't think that's possible since the task is posted, > and it would get a new stack frame, and the thread_id_ would be set by then? I originally added it to check that no other thread modifies the task runner while the first task is being scheduled. I removed it now. https://codereview.chromium.org/637983003/diff/220001/base/timer/timer.cc#new... base/timer/timer.cc:98: DCHECK(thread_id_ == 0); On 2014/10/18 19:00:00, danakj wrote: > use DCHECK_EQ Done. https://codereview.chromium.org/637983003/diff/220001/base/timer/timer.cc#new... base/timer/timer.cc:171: return task_runner_.get() == nullptr ? On 2014/10/18 19:00:00, danakj wrote: > == nullptr is redundant here > > return task_runner_.get() ? task_runner_ : RunnerHandle::Get() ? Done. https://codereview.chromium.org/637983003/diff/220001/base/timer/timer.h File base/timer/timer.h (right): https://codereview.chromium.org/637983003/diff/220001/base/timer/timer.h#newc... base/timer/timer.h:131: // corresponding task_runner_ field is NULL, the task runner for the current On 2014/10/18 19:00:00, danakj wrote: > NULL -> empty or null? Done (null). https://codereview.chromium.org/637983003/diff/220001/base/timer/timer.h#newc... base/timer/timer.h:152: // The task runner on which the task should be scheduled. If it is NULL, the On 2014/10/18 19:00:00, danakj wrote: > NULL -> empty or null? Done (null).
LGTM https://codereview.chromium.org/637983003/diff/240001/base/timer/timer.h File base/timer/timer.h (right): https://codereview.chromium.org/637983003/diff/240001/base/timer/timer.h#newc... base/timer/timer.h:58: #include "base/single_thread_task_runner.h" can this be forward declared in this header? https://codereview.chromium.org/637983003/diff/240001/base/timer/timer_unitte... File base/timer/timer_unittest.cc (right): https://codereview.chromium.org/637983003/diff/240001/base/timer/timer_unitte... base/timer/timer_unittest.cc:38: void SetTaskRunner(scoped_refptr<SingleThreadTaskRunner> task_runner) { nit: should be 1 whitespace above/below the method https://codereview.chromium.org/637983003/diff/240001/base/timer/timer_unitte... base/timer/timer_unittest.cc:333: nit: drop whitespace please https://codereview.chromium.org/637983003/diff/240001/base/timer/timer_unitte... base/timer/timer_unittest.cc:335: ditto
Thanks for your comments. I will now commit the patch. https://codereview.chromium.org/637983003/diff/240001/base/timer/timer.h File base/timer/timer.h (right): https://codereview.chromium.org/637983003/diff/240001/base/timer/timer.h#newc... base/timer/timer.h:58: #include "base/single_thread_task_runner.h" On 2014/10/22 15:07:43, danakj wrote: > can this be forward declared in this header? Done. https://codereview.chromium.org/637983003/diff/240001/base/timer/timer_unitte... File base/timer/timer_unittest.cc (right): https://codereview.chromium.org/637983003/diff/240001/base/timer/timer_unitte... base/timer/timer_unittest.cc:38: void SetTaskRunner(scoped_refptr<SingleThreadTaskRunner> task_runner) { On 2014/10/22 15:07:44, danakj wrote: > nit: should be 1 whitespace above/below the method Done (for all methods). https://codereview.chromium.org/637983003/diff/240001/base/timer/timer_unitte... base/timer/timer_unittest.cc:333: On 2014/10/22 15:07:44, danakj wrote: > nit: drop whitespace please Done. https://codereview.chromium.org/637983003/diff/240001/base/timer/timer_unitte... base/timer/timer_unittest.cc:335: On 2014/10/22 15:07:44, danakj wrote: > ditto Done.
The CQ bit was checked by petrcermak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/637983003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
petrcermak@chromium.org changed reviewers: + mseaborn@chromium.org, thestig@chromium.org
Hi Lei and Mark, Could you please help us diagnose the native client trybot failures happening with this patch? In all cases, the linker fails to find 'base::Timer::SetTaskRunner(scoped_refptr<base::SingleThreadTaskRunner>)' in ../out/Release/gen/tc_pnacl_newlib/lib/libremoting_client_plugin_lib_nacl.a. We found that once we clean our workspace (e.g. using "cr clean"), we are unable to reproduce this failure. Therefore, it seems that the failures are caused by the build files. Thanks, Petr
On 24 October 2014 07:58, <petrcermak@chromium.org> wrote: > Hi Lei and Mark, > > Could you please help us diagnose the native client trybot failures > happening > with this patch? > > In all cases, the linker fails to find > 'base::Timer::SetTaskRunner(scoped_refptr<base::SingleThreadTaskRunner>)' > in > ../out/Release/gen/tc_pnacl_newlib/lib/libremoting_client_plugin_lib_nacl.a. > We > found that once we clean our workspace (e.g. using "cr clean"), we are > unable to > reproduce this failure. Therefore, it seems that the failures are caused > by the > build files. > Hmm, from the build log I see you are getting 2 rebuilds of the base library for NaCl: [289/6286 | 10.066] ACTION base_nacl: build IRT x86-32 nlib_9b4f41e4158ebb93a5d28e6734a13e85 [291/6286 | 10.079] ACTION base_nacl_nonsfi: build nonsfi_helper x86-32 nlib_d8abab5c18b27956fdcb69ad796a5734 (from http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... ) but you're not getting the one you need, which I think would be "ACTION base_nacl: build newlib plib". The dependencies in remoting_nacl.gyp look correct. You could try inspecting the Ninja dependencies that Gyp generates for: * base_nacl (plib version): does it declare dependencies on the source files properly? * 'remoting_client_plugin_nacl': does it declare a dependency on the .a library file properly? Cheers, Mark To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 24 October 2014 09:00, Mark Seaborn <mseaborn@chromium.org> wrote: > On 24 October 2014 07:58, <petrcermak@chromium.org> wrote: > >> Hi Lei and Mark, >> >> Could you please help us diagnose the native client trybot failures >> happening >> with this patch? >> >> In all cases, the linker fails to find >> 'base::Timer::SetTaskRunner(scoped_refptr<base::SingleThreadTaskRunner>)' >> in >> ../out/Release/gen/tc_pnacl_newlib/lib/libremoting_client_plugin_lib_nacl.a. >> We >> found that once we clean our workspace (e.g. using "cr clean"), we are >> unable to >> reproduce this failure. Therefore, it seems that the failures are caused >> by the >> build files. >> > > Hmm, from the build log I see you are getting 2 rebuilds of the base > library for NaCl: > > [289/6286 | 10.066] ACTION base_nacl: build IRT x86-32 > nlib_9b4f41e4158ebb93a5d28e6734a13e85 > [291/6286 | 10.079] ACTION base_nacl_nonsfi: build nonsfi_helper x86-32 > nlib_d8abab5c18b27956fdcb69ad796a5734 > > (from > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... > ) > > but you're not getting the one you need, which I think would be "ACTION > base_nacl: build newlib plib". > > The dependencies in remoting_nacl.gyp look correct. > > You could try inspecting the Ninja dependencies that Gyp generates for: > * base_nacl (plib version): does it declare dependencies on the source > files properly? > That seems to be where the problem lies. I had a look at the Ninja output. See out/Debug/obj/base/base_nacl.ninja. Compare these declarations: "build gen/tc_pnacl_newlib/lib/libbase_nacl.a" -- lists only 1 screenful of files, omitting base/timer/timer.cc "build gen/tc_irt/lib64/libbase_nacl.a" -- lists many more files, including base/timer/timer.cc There's probably a bug in NaCl's untrusted.gypi, but I haven't worked out what yet. Cheers, Mark To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 24 October 2014 11:32, Mark Seaborn <mseaborn@chromium.org> wrote: > On 24 October 2014 09:00, Mark Seaborn <mseaborn@chromium.org> wrote: > >> On 24 October 2014 07:58, <petrcermak@chromium.org> wrote: >> >>> Hi Lei and Mark, >>> >>> Could you please help us diagnose the native client trybot failures >>> happening >>> with this patch? >>> >>> In all cases, the linker fails to find >>> 'base::Timer::SetTaskRunner(scoped_refptr<base::SingleThreadTaskRunner>)' >>> in >>> ../out/Release/gen/tc_pnacl_newlib/lib/libremoting_client_plugin_lib_nacl.a. >>> We >>> found that once we clean our workspace (e.g. using "cr clean"), we are >>> unable to >>> reproduce this failure. Therefore, it seems that the failures are caused >>> by the >>> build files. >>> >> >> Hmm, from the build log I see you are getting 2 rebuilds of the base >> library for NaCl: >> >> [289/6286 | 10.066] ACTION base_nacl: build IRT x86-32 >> nlib_9b4f41e4158ebb93a5d28e6734a13e85 >> [291/6286 | 10.079] ACTION base_nacl_nonsfi: build nonsfi_helper x86-32 >> nlib_d8abab5c18b27956fdcb69ad796a5734 >> >> (from >> http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... >> ) >> >> but you're not getting the one you need, which I think would be "ACTION >> base_nacl: build newlib plib". >> >> The dependencies in remoting_nacl.gyp look correct. >> >> You could try inspecting the Ninja dependencies that Gyp generates for: >> * base_nacl (plib version): does it declare dependencies on the source >> files properly? >> > > That seems to be where the problem lies. I had a look at the Ninja > output. See out/Debug/obj/base/base_nacl.ninja. > > Compare these declarations: > > "build gen/tc_pnacl_newlib/lib/libbase_nacl.a" -- lists only 1 screenful > of files, omitting base/timer/timer.cc > "build gen/tc_irt/lib64/libbase_nacl.a" -- lists many more files, > including base/timer/timer.cc > > There's probably a bug in NaCl's untrusted.gypi, but I haven't worked out > what yet. > I instrumented native_client/build/scan_sources.py to investigate what's going on. The "build newlib plib" action includes this in the "inputs" list: '>!@pymod_do_main(scan_sources -I . >(include_dirs) >(_include_dirs) -S >(sources) >(_sources))', "^(sources)" expands correctly to: 'base_switches.cc', 'base_switches.h', 'strings/string16.cc', 'sync_socket_nacl.cc', 'time/time_posix.cc' (the list from "sources" in base/base_nacl.gyp). The problem is that "^(_sources)" is expanding to an empty list. However, in the "build irt x86-64 nlib" action, "^(_sources)" is expanding correctly to the full list (from "sources" in base/base.gypi). I'm not sure what the difference is between these two actions that causes the plib action to break. Cheers, Mark To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 24 October 2014 16:59, Mark Seaborn <mseaborn@chromium.org> wrote: > On 24 October 2014 11:32, Mark Seaborn <mseaborn@chromium.org> wrote: > >> On 24 October 2014 09:00, Mark Seaborn <mseaborn@chromium.org> wrote: >> >>> On 24 October 2014 07:58, <petrcermak@chromium.org> wrote: >>> >>>> Hi Lei and Mark, >>>> >>>> Could you please help us diagnose the native client trybot failures >>>> happening >>>> with this patch? >>>> >>>> In all cases, the linker fails to find >>>> 'base::Timer::SetTaskRunner(scoped_refptr<base::SingleThreadTaskRunner>)' >>>> in >>>> ../out/Release/gen/tc_pnacl_newlib/lib/libremoting_client_plugin_lib_nacl.a. >>>> We >>>> found that once we clean our workspace (e.g. using "cr clean"), we are >>>> unable to >>>> reproduce this failure. Therefore, it seems that the failures are >>>> caused by the >>>> build files. >>>> >>> >>> Hmm, from the build log I see you are getting 2 rebuilds of the base >>> library for NaCl: >>> >>> [289/6286 | 10.066] ACTION base_nacl: build IRT x86-32 >>> nlib_9b4f41e4158ebb93a5d28e6734a13e85 >>> [291/6286 | 10.079] ACTION base_nacl_nonsfi: build nonsfi_helper x86-32 >>> nlib_d8abab5c18b27956fdcb69ad796a5734 >>> >>> (from >>> http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... >>> ) >>> >>> but you're not getting the one you need, which I think would be "ACTION >>> base_nacl: build newlib plib". >>> >>> The dependencies in remoting_nacl.gyp look correct. >>> >>> You could try inspecting the Ninja dependencies that Gyp generates for: >>> * base_nacl (plib version): does it declare dependencies on the source >>> files properly? >>> >> >> That seems to be where the problem lies. I had a look at the Ninja >> output. See out/Debug/obj/base/base_nacl.ninja. >> >> Compare these declarations: >> >> "build gen/tc_pnacl_newlib/lib/libbase_nacl.a" -- lists only 1 screenful >> of files, omitting base/timer/timer.cc >> "build gen/tc_irt/lib64/libbase_nacl.a" -- lists many more files, >> including base/timer/timer.cc >> >> There's probably a bug in NaCl's untrusted.gypi, but I haven't worked out >> what yet. >> > > I instrumented native_client/build/scan_sources.py to investigate what's > going on. > > The "build newlib plib" action includes this in the "inputs" list: > > '>!@pymod_do_main(scan_sources -I . >(include_dirs) >(_include_dirs) -S > >(sources) >(_sources))', > > "^(sources)" expands correctly to: 'base_switches.cc', 'base_switches.h', > 'strings/string16.cc', 'sync_socket_nacl.cc', 'time/time_posix.cc' (the > list from "sources" in base/base_nacl.gyp). > > The problem is that "^(_sources)" is expanding to an empty list. > > However, in the "build irt x86-64 nlib" action, "^(_sources)" is expanding > correctly to the full list (from "sources" in base/base.gypi). I'm not > sure what the difference is between these two actions that causes the plib > action to break. > Petr, can you file an issue about this, please, and I'll try to route it to someone who knows how to debug Gyp issues better than I do? Cheers, Mark To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/10/24 14:58:47, petrcermak wrote: > Hi Lei and Mark, > > Could you please help us diagnose the native client trybot failures happening > with this patch? > > In all cases, the linker fails to find > 'base::Timer::SetTaskRunner(scoped_refptr<base::SingleThreadTaskRunner>)' in > ../out/Release/gen/tc_pnacl_newlib/lib/libremoting_client_plugin_lib_nacl.a. We > found that once we clean our workspace (e.g. using "cr clean"), we are unable to > reproduce this failure. Therefore, it seems that the failures are caused by the > build files. > > Thanks, > Petr I tried replicating the linux_chromium_compile_dbg_32 build environment but couldn't repro. I even unapplied your patch, built, reapplied your patch, and built again.
On 2014/10/25 01:53:09, Mark Seaborn wrote: > On 24 October 2014 16:59, Mark Seaborn <mailto:mseaborn@chromium.org> wrote: > > > On 24 October 2014 11:32, Mark Seaborn <mailto:mseaborn@chromium.org> wrote: > > > >> On 24 October 2014 09:00, Mark Seaborn <mailto:mseaborn@chromium.org> wrote: > >> > >>> On 24 October 2014 07:58, <mailto:petrcermak@chromium.org> wrote: > >>> > >>>> Hi Lei and Mark, > >>>> > >>>> Could you please help us diagnose the native client trybot failures > >>>> happening > >>>> with this patch? > >>>> > >>>> In all cases, the linker fails to find > >>>> 'base::Timer::SetTaskRunner(scoped_refptr<base::SingleThreadTaskRunner>)' > >>>> in > >>>> > ../out/Release/gen/tc_pnacl_newlib/lib/libremoting_client_plugin_lib_nacl.a. > >>>> We > >>>> found that once we clean our workspace (e.g. using "cr clean"), we are > >>>> unable to > >>>> reproduce this failure. Therefore, it seems that the failures are > >>>> caused by the > >>>> build files. > >>>> > >>> > >>> Hmm, from the build log I see you are getting 2 rebuilds of the base > >>> library for NaCl: > >>> > >>> [289/6286 | 10.066] ACTION base_nacl: build IRT x86-32 > >>> nlib_9b4f41e4158ebb93a5d28e6734a13e85 > >>> [291/6286 | 10.079] ACTION base_nacl_nonsfi: build nonsfi_helper x86-32 > >>> nlib_d8abab5c18b27956fdcb69ad796a5734 > >>> > >>> (from > >>> > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... > >>> ) > >>> > >>> but you're not getting the one you need, which I think would be "ACTION > >>> base_nacl: build newlib plib". > >>> > >>> The dependencies in remoting_nacl.gyp look correct. > >>> > >>> You could try inspecting the Ninja dependencies that Gyp generates for: > >>> * base_nacl (plib version): does it declare dependencies on the source > >>> files properly? > >>> > >> > >> That seems to be where the problem lies. I had a look at the Ninja > >> output. See out/Debug/obj/base/base_nacl.ninja. > >> > >> Compare these declarations: > >> > >> "build gen/tc_pnacl_newlib/lib/libbase_nacl.a" -- lists only 1 screenful > >> of files, omitting base/timer/timer.cc > >> "build gen/tc_irt/lib64/libbase_nacl.a" -- lists many more files, > >> including base/timer/timer.cc > >> > >> There's probably a bug in NaCl's untrusted.gypi, but I haven't worked out > >> what yet. > >> > > > > I instrumented native_client/build/scan_sources.py to investigate what's > > going on. > > > > The "build newlib plib" action includes this in the "inputs" list: > > > > '>!@pymod_do_main(scan_sources -I . >(include_dirs) >(_include_dirs) -S > > >(sources) >(_sources))', > > > > "^(sources)" expands correctly to: 'base_switches.cc', 'base_switches.h', > > 'strings/string16.cc', 'sync_socket_nacl.cc', 'time/time_posix.cc' (the > > list from "sources" in base/base_nacl.gyp). > > > > The problem is that "^(_sources)" is expanding to an empty list. > > > > However, in the "build irt x86-64 nlib" action, "^(_sources)" is expanding > > correctly to the full list (from "sources" in base/base.gypi). I'm not > > sure what the difference is between these two actions that causes the plib > > action to break. > > > > Petr, can you file an issue about this, please, and I'll try to route it to > someone who knows how to debug Gyp issues better than I do? > > Cheers, > Mark > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Thank you for your help. I filed a bug about this: https://code.google.com/p/chromium/issues/detail?id=427427
Patchset #6 (id:280001) has been deleted
The CQ bit was checked by petrcermak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/637983003/340001
Message was sent while issue was closed.
Committed patchset #8 (id:340001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/7652da6d5cbd7e5b82b340e638ebe80aafeacf6e Cr-Commit-Position: refs/heads/master@{#302947} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
