Closed
Bug 1263212
Opened 9 years ago
Closed 7 years ago
Simplify the JobScheduler API
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: nical, Assigned: nical)
References
Details
(Keywords: feature)
Attachments
(2 files)
21.90 KB,
patch
|
Details | Diff | Splinter Review | |
10.22 KB,
patch
|
Details | Diff | Splinter Review |
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().
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8739495 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 2•9 years ago
|
||
Green try push with work stealing and this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e45c35e7d499fb2f150a39dfe32ccb4235f8c0e1
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).
Attachment #8744331 -
Flags: review?(jmuizelaar)
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.
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.
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.
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).
Assignee | ||
Updated•7 years ago
|
Attachment #8739495 -
Flags: review?(jmuizelaar)
Assignee | ||
Updated•7 years ago
|
Attachment #8744331 -
Flags: review?(jmuizelaar)
Assignee | ||
Updated•7 years ago
|
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•