[3/9] bitbake/runqueue: initialize RunQueueExecute before printdiff rather than after

printdiff needs setscene dependencies and they're available only
through that object. Previously it was instantianted just after
running printdiff, this moves the initilization to just prior.

Signed-off-by: Alexander Kanavin <alex@linutronix.de>
---
 bitbake/lib/bb/runqueue.py | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

On Thu, 2023-12-14 at 14:45 +0100, Alexander Kanavin wrote:
> printdiff needs setscene dependencies and they're available only
> through that object. Previously it was instantianted just after
> running printdiff, this moves the initilization to just prior.
> 
> Signed-off-by: Alexander Kanavin <alex@linutronix.de>
> ---
>  bitbake/lib/bb/runqueue.py | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/bitbake/lib/bb/runqueue.py b/bitbake/lib/bb/runqueue.py
> index 864708ee4a5..1f59d18b71a 100644
> --- a/bitbake/lib/bb/runqueue.py
> +++ b/bitbake/lib/bb/runqueue.py
> @@ -1555,6 +1555,11 @@ class RunQueue:
>                                           ('bb.event.HeartbeatEvent',), data=self.cfgData)
>                   self.dm_event_handler_registered = True
>  
> +            self.rqdata.init_progress_reporter.next_stage()
> +            self.start_worker()
> +            self.rqdata.init_progress_reporter.next_stage()
> +            self.rqexe = RunQueueExecute(self)
> +
>              dump = self.cooker.configuration.dump_signatures
>              if dump:
>                  self.rqdata.init_progress_reporter.finish()
> @@ -1566,11 +1571,6 @@ class RunQueue:
>                  self.state = runQueueComplete
>  
>          if self.state is runQueueSceneInit:
> -            self.rqdata.init_progress_reporter.next_stage()
> -            self.start_worker()
> -            self.rqdata.init_progress_reporter.next_stage()
> -            self.rqexe = RunQueueExecute(self)
> -
>              # If we don't have any setscene functions, skip execution
>              if not self.rqdata.runq_setscene_tids:
>                  logger.info('No setscene tasks')

I not entirely happy about this since start_worker() executes processes
and isn't trivial. The code is careful enough to tear them down too at
exit but it is all a bit of a waste of time.

I had wondered if we can create RunQueueExecute() without the workers
but as the code stands, it does poke things into them in a small
isolated section. I think this code flow should be tweaked to stop
RunQueueExecute needing the workers to be started and that would be a
decent cleanup of the code anyway.

I can take a look at that if it helps since I think I've been moving
towards that refactor for a while anyway?

Cheers,

Richard

On Thu, 14 Dec 2023 at 15:39, Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:

> I not entirely happy about this since start_worker() executes processes
> and isn't trivial. The code is careful enough to tear them down too at
> exit but it is all a bit of a waste of time.
>
> I had wondered if we can create RunQueueExecute() without the workers
> but as the code stands, it does poke things into them in a small
> isolated section. I think this code flow should be tweaked to stop
> RunQueueExecute needing the workers to be started and that would be a
> decent cleanup of the code anyway.
>
> I can take a look at that if it helps since I think I've been moving
> towards that refactor for a while anyway?

Yes please. I haven't noticed any regression in printdiff performance,
other than a couple of additional lines printed, but if you can make a
better patch, that'd be welcome.

Meanwhile I'd like to implement what I mentioned in note (1) in patch
4/9, as it's a real regression that I realized only today as I was
preparing the patchset for submission and now it's bothering me :)

Alex

On Thu, 2023-12-14 at 18:28 +0100, Alexander Kanavin wrote:
> On Thu, 14 Dec 2023 at 15:39, Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
> 
> > I not entirely happy about this since start_worker() executes processes
> > and isn't trivial. The code is careful enough to tear them down too at
> > exit but it is all a bit of a waste of time.
> > 
> > I had wondered if we can create RunQueueExecute() without the workers
> > but as the code stands, it does poke things into them in a small
> > isolated section. I think this code flow should be tweaked to stop
> > RunQueueExecute needing the workers to be started and that would be a
> > decent cleanup of the code anyway.
> > 
> > I can take a look at that if it helps since I think I've been moving
> > towards that refactor for a while anyway?
> 
> Yes please. I haven't noticed any regression in printdiff performance,
> other than a couple of additional lines printed, but if you can make a
> better patch, that'd be welcome.

Patch on the bitbake list for this which removes more code than
it adds. We had already set everything up to do this :)

> Meanwhile I'd like to implement what I mentioned in note (1) in patch
> 4/9, as it's a real regression that I realized only today as I was
> preparing the patchset for submission and now it's bothering me :)

Yes, it is and I do agree we need to fix that.

Cheers,

Richard

On Fri, 15 Dec 2023 at 17:04, Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:

> > Meanwhile I'd like to implement what I mentioned in note (1) in patch
> > 4/9, as it's a real regression that I realized only today as I was
> > preparing the patchset for submission and now it's bothering me :)
>
> Yes, it is and I do agree we need to fix that.

I just completed the fixing. It all works wonderfully in local builds,
but who knows what situations AB will be able to come up with :) It's
a bit crowded on the AB right now to start the tests there, so here
are the patches:
https://git.yoctoproject.org/poky-contrib/log/?h=akanavin/fix-printdiff
(I run the tests with a special contrib/kanavin/ branch of
autobuilder-helper that only runs the three needed selftests, and not
the whole gigantic set)

I also reverted the gnu-config tweak in that branch as it should be no
longer necessary.

I'll cherry-pick the bitbake patch before doing any further tests, but
it's now Friday evening, so anxiously watching ongoing selftests is
perhaps not the best idea.

Alex

On Fri, 2023-12-15 at 17:49 +0100, Alexander Kanavin wrote:
> On Fri, 15 Dec 2023 at 17:04, Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
> 
> > > Meanwhile I'd like to implement what I mentioned in note (1) in patch
> > > 4/9, as it's a real regression that I realized only today as I was
> > > preparing the patchset for submission and now it's bothering me :)
> > 
> > Yes, it is and I do agree we need to fix that.
> 
> I just completed the fixing. It all works wonderfully in local builds,
> but who knows what situations AB will be able to come up with :) It's
> a bit crowded on the AB right now to start the tests there, so here
> are the patches:
> https://git.yoctoproject.org/poky-contrib/log/?h=akanavin/fix-printdiff
> (I run the tests with a special contrib/kanavin/ branch of
> autobuilder-helper that only runs the three needed selftests, and not
> the whole gigantic set)

Sounds good. I think we are getting to the bottom of some of the
underlying issues which is good and longer term should help usability a
lot.

> I also reverted the gnu-config tweak in that branch as it should be no
> longer necessary.

I was torn on that. I can drop from -next too.

> I'll cherry-pick the bitbake patch before doing any further tests, but
> it's now Friday evening, so anxiously watching ongoing selftests is
> perhaps not the best idea.

Fair enough! I've run that patch through local only testing so far, as
you say, the AB has a lot going on.

I've been torn on waiting for these or building M1 but I'm aiming for
just the CDN fix since this may or may not take a while to settle on
the autobuilder and I don't think we need any more pressure. The
failures are driving Alexandre a bit crazy in swatbot so getting things
sorted is important.

Cheers,

Richard