Nicolas Silva [:nical] Assignee
	Description • 9 years ago

I made a few tweaks to the job scheduler:
 - Remove the JobStatus enum returned by every job (we don't actually use it) and have jobs return void. It was a poor way to handle errors (we would just MOZ_CRASH if it was anything other than JobStatus::Complete).
 - remove the "start" sync object from the Job class. It was only used in JobScheduler::Submit. It's actually nicer to have this at the API level and saves the extra pointer in Job.
 - JobScheduler::SubmitJob now always schedules jobs (with no dependency). Use syncObject->Then(job) to express that job has to be run after syncObject is signaled. This let me remove the error-prone SyncObject::RegisterJob method that would conditionally take ownership of the job.
 - In the same vein, JobScheduler::Join(SyncObject*) moved into syncObject->Join().

	Nicolas Silva [:nical] Assignee
	Comment 2 • 9 years ago

Green try push with work stealing and this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e45c35e7d499fb2f150a39dfe32ccb4235f8c0e1

	Nicolas Silva [:nical] Assignee
	Comment 3 • 9 years ago

Event objects appear to be fairly expensive to create on windows. This patch changes the api a bit so that we can more easily reuse them. The construction of the object is done in a static Create() method in which we can eventually implement recycling a pool of event objects if need be (applied the same to SyncObject for consistency).

	Jeff Muizelaar [:jrmuizel]
	Comment 4 • 9 years ago

(In reply to Nicolas Silva [:nical] from comment #3)
> Created attachment 8744331 [details] [diff] [review]
> Make EventObject and SyncObject more easily reusable.
> 
> Event objects appear to be fairly expensive to create on windows. This patch
> changes the api a bit so that we can more easily reuse them. The
> construction of the object is done in a static Create() method in which we
> can eventually implement recycling a pool of event objects if need be
> (applied the same to SyncObject for consistency).

I'd rather we just do the reusing part with out the ::Create() part. I still prefer explicit reuse instead of a recycling pool. If we do need the recycling pool we can always add ::Create then.

	Jeff Muizelaar [:jrmuizel]
	Comment 5 • 9 years ago

Comment on attachment 8739495 [details] [diff] [review]
Patch.

Review of attachment 8739495 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/JobScheduler.cpp
@@ +174,5 @@
>    // we specify the number of prerequisites in the constructor, but in the typical
>    // scenario, if the assertion FreezePrerequisite blows up here it probably means
>    // we got the initial nmber of prerequisites wrong. We can decide to remove
>    // this restriction if needed.
> +  //FreezePrerequisites();

Is this intentional?

@@ +203,5 @@
> +void
> +SyncObject::Join()
> +{
> +  FreezePrerequisites();
> +  RefPtr<EventObject> waitForCompletion = new EventObject();

My gut reaction to having Join allocate a new EventObject (given their expensiveness) is negative. I don't have an alternative suggestion yet though.

	Jeff Muizelaar [:jrmuizel]
	Comment 6 • 9 years ago

In theory we should only need one OS synchronization primitive per thread. It might be worth seeing if we can push the design more in that direction.

	Nicolas Silva [:nical] Assignee
	Comment 7 • 9 years ago

(In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> > +  //FreezePrerequisites();
> 
> Is this intentional?

Yeah I think so (I wrote this a while back). Basically the assertion is not really needed but I originally felt like it was "good practice" to not add new prerequisites after you start adding subsequents to a sync object, but it wouldn't actually create bugs (since we specify the number of prerequisites in advance), the restriction is completely artificial. I guess I changed my mind while playing around with the api and the tests. I should have removed the assertion rather than commented it out, though.

> My gut reaction to having Join allocate a new EventObject (given their
> expensiveness) is negative. I don't have an alternative suggestion yet
> though.

The other patch is intended to mitigate that by letting us avoid the EventObject allocation either by passing one as a parameter to Join, or by letting us recycle EventObjects transparently (that would be hidden behind EventObject::Create() although it's not actually implemented).