GNOME Bugzilla – Bug 577772
Revamp the job scheduling (Banshee.Kernel and UserJob*)
Last modified: 2020-03-17 08:26:58 UTC
The existing Banshee.Kernel and UserJob* code should be unified and improved. IF possible, all jobs should use the db for their state - eg for a write-metadata-to-file job, it would: 1) keep track of when it last saved data to disk 2) select a track that where CoreTracks.UpdatedAt > last-write-time and write it out 3) repeat 2 until no more This gives us the huge advantage of being able to restart on the next run such tasks if the user quits before we're done. Also, we could have some smart scheduling to improve performance. I've got some code for this started in the jobs branch of http://banshee-project.org/~gburt/banshee.git
The new Hyena.Jobs code is in git master, and many jobs are changed over to using it, but not all - Banshee.Kernel is still around/in use.
Created attachment 173991 [details] [review] move to hyena jobs here is a patch to use hyena jobs. If all work Banshee.Kernel can be removed. I have test with podcasts but need new sound to get new covert art.
Review of attachment 173991 [details] [review]: Banshee.Kernel would execute jobs serially on a thread, where Hyena.Jobs.Scheduler will attempt to run jobs in parallel, and does no threading work itself. Whatever thread schedule.Add is called from is the thread the jobs will run on, unless that jobs does its own threading or inherits from SimpleAsyncJob. We should probably add a "Network" resource to these jobs to ensure they run serially, and should probably have MetadataServiceJob inherit from SimpleAsyncJob. ::: src/Core/Banshee.Services/Banshee.Metadata/MetadataService.cs @@ +157,2 @@ { + Job job = sender as Job; Should stop listening to job.Finished here ::: src/Core/Banshee.Services/Banshee.Metadata/MetadataServiceJob.cs @@ +78,1 @@ { Should add a call to Hyena.ThreadAssist.AssertNotInMainThread () here and then test to make sure this isn't getting called from the main/GUI thread. Banshee.Kernel executed its items serially on a different thread, but Hyena.Jobs doesn't do that automatically for you -- jobs have to do their own threading (and/or inherit from SimpleAsyncJob).
Created attachment 174153 [details] [review] move metadata service job to hyena jobs OK, I have done a new patch with your feedback. I do not set resource Network for FileSystemQueryJob by not calling base() ctor whereas I call it on all other metadataServiceJob inherited class. is it ok or FileSystemQueryJob need to be with resource too?
Review of attachment 174153 [details] [review]: Thanks Olivier, looking better. I wish a few things were different: 1) We use the normal job scheduler (ServiceManager.JobScheduler) 2) The only job we add to the scheduler is a MetadataJob : SimpleAsyncJob, that in its Run method loops through the various service/job classes serially. The service/jobs should inherit from some simple class or interface, not be Hyena.Jobs, etc. Sorry, not trying to dump on you. Just trying to brain-dump why I'm hesitant to accept this patch (or work on this part of the code at all -- it's way too complex for what it does).
Created attachment 175505 [details] [review] patch revamp metadata jobs Ok, this time, I have done a big refactoring to simplify chain of inheritance... I have cut the job class in 2. the LookupJob and the scheduller job. Hope you will enjoy design and simplification for maintainability. Can you review it ? It will need a lot of test to not break or add regression.
Created attachment 175506 [details] [review] patch revamp metadata jobs oops, wrong file (forget to add some files)
To simplify, I think to merge lookupjob into provider. So we will end with: Service just add regular metadataServiceJob on scheduler which will loop on provider to get tags and cover. I will rename List<StreamTag> Run () to "Process ()" to avoid confusing with job Run method and IMetadataLookupJob will be merge with IMetadataProvider. Are you OK with design ? public interface IMetadataProvider { IBasicTrackInfo Track { get; } IEnumerable<StreamTag> Process (); void Cancel (IBasicTrackInfo track); void CancelAll (); }
Hmm cancel must be moved to Service and removed from MetadataProvider too... So interface become: public interface IMetadataProvider { IBasicTrackInfo Track { get; } IEnumerable<StreamTag> Process (); }
Created attachment 175566 [details] [review] patch to simplify metadata service Ok, I have finished my refactoring idea. Can you take a look, it is far better... I have remove the job mixed up things and even remove the need of couple provider/lookupJob to just need of provider which contain all. It is not tested but it compile and it is just to have feedback to know if go in the good direction...before debuging/testing
Olivier, looks like you forgot to `git add` PodcastImageMetadataProvider.cs
Created attachment 176112 [details] [review] patch revamp metadata jobs 2 here I add the missing file...
Review of attachment 176112 [details] [review]: Does not apply on banshee 2.2 master: git apply 0001-Metadata-Jobs-Simplify-Job-system-and-use-regular-sc.patch 0001-Metadata-Jobs-Simplify-Job-system-and-use-regular-sc.patch:1329: trailing whitespace. // 0001-Metadata-Jobs-Simplify-Job-system-and-use-regular-sc.patch:1331: trailing whitespace. // 0001-Metadata-Jobs-Simplify-Job-system-and-use-regular-sc.patch:1358: trailing whitespace. // 0001-Metadata-Jobs-Simplify-Job-system-and-use-regular-sc.patch:1359: trailing whitespace. // Copyright 2010 0001-Metadata-Jobs-Simplify-Job-system-and-use-regular-sc.patch:1360: trailing whitespace. // error: patch failed: src/Core/Banshee.Services/Banshee.Metadata.MusicBrainz/MusicBrainzQueryJob.cs:1 error: src/Core/Banshee.Services/Banshee.Metadata.MusicBrainz/MusicBrainzQueryJob.cs: patch does not apply error: patch failed: src/Core/Banshee.Services/Banshee.Metadata.Rhapsody/RhapsodyQueryJob.cs:61 error: src/Core/Banshee.Services/Banshee.Metadata.Rhapsody/RhapsodyQueryJob.cs: patch does not apply error: patch failed: src/Core/Banshee.Services/Banshee.Services.csproj:70 error: src/Core/Banshee.Services/Banshee.Services.csproj: patch does not apply error: patch failed: src/Extensions/Banshee.Podcasting/Banshee.Podcasting.csproj:120 error: src/Extensions/Banshee.Podcasting/Banshee.Podcasting.csproj: patch does not apply
Banshee is not under active development anymore and had its last code changes more than three years ago. Its codebase has been archived. Closing this report as WONTFIX as part of Bugzilla Housekeeping to reflect reality. Please feel free to reopen this ticket (or rather transfer the project to GNOME Gitlab, as GNOME Bugzilla is being shut down) if anyone takes the responsibility for active development again. See https://gitlab.gnome.org/Infrastructure/Infrastructure/issues/264 for more info.