Closed
Bug 1057261
Opened 10 years ago
Closed 10 years ago
Cache preference value in GonkHal::SetProcessPriority
Categories
(Core :: Hardware Abstraction Layer (HAL), defect)
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: kanru, Assigned: gsvelto)
Details
Attachments
(1 file, 4 obsolete files)
9.30 KB,
patch
|
gsvelto
:
review+
|
Details | Diff | Splinter Review |
Currently when calling SetProcessPriority we will read two value from preference. This is not a infrequently used function so I think we should cache those preferences.
Assignee | ||
Comment 1•10 years ago
|
||
+1, let's do that. Keep in mind however that this likely needs to be done lazily since when calling the functions we don't know if the preference service has already been initialized or not IIRC.
Assignee | ||
Comment 2•10 years ago
|
||
Here's a crude patch I quickly hacked together today.
Assignee | ||
Comment 3•10 years ago
|
||
This is a cleaned up patch that also covers thread priorities. I verified that it works correctly but didn't have time to check what difference it makes performance-wise when calling these methods.
Assignee | ||
Comment 4•10 years ago
|
||
Just an additional note on the patch here. Previously we crashed with MOZ_ASSERT() if the preferences weren't set. Instead I've used the Add*VarCache() functions for reading the prefs; this ensures that the relevant variables are automatically updated when the associated prefs change and that they're set to a safe default (0) if the pref is not present. If we want the previous behavior it's OK by me but it seemed overkill.
Comment 5•10 years ago
|
||
Comment on attachment 8494380 [details] [diff] [review] [PATCH] Cache the preferences affecting process and thread priorities Review of attachment 8494380 [details] [diff] [review]: ----------------------------------------------------------------- Just a bunch of minor stuff. Overall, the algorithm looks good. ::: hal/gonk/GonkHal.cpp @@ +1490,5 @@ > + */ > +struct ProcessPriorityPrefs { > + bool initialized; > + int nice[NUM_PROCESS_PRIORITY]; > + int oomScoreAdj[NUM_PROCESS_PRIORITY]; nit: If nice and oomScoreAdj are tightly coupled to each other (i.e. per priority) then I prefer to see a struct and have an array of structs, rather than 2 separate arrays. So you would use prefs->prio[i].nice and rather then prefs->nice[i] (using an appropriate word for prio). @@ +1500,5 @@ > + * watchers so that if they're dynamically changed the change is reflected on > + * the appropriate variables. > + */ > +void > +EnsureProcessPriorityPrefs(ProcessPriorityPrefs *prefs) nit: move star to left @@ +1511,5 @@ > + for (int i = PROCESS_PRIORITY_BACKGROUND; i < NUM_PROCESS_PRIORITY; i++) { > + ProcessPriority priority = static_cast<ProcessPriority>(i); > + > + // Read the nice values > + const char *processPriorityStr = ProcessPriorityToString(priority); nit: move star left @@ +1513,5 @@ > + > + // Read the nice values > + const char *processPriorityStr = ProcessPriorityToString(priority); > + nsCString str = nsPrintfCString("hal.processPriorityManager.gonk.%s.Nice", > + processPriorityStr); Shouldn't you just declare: nsPrintfCString str("hal.processPriorityManager.gonk.%s.Nice", processPriorityStr); @@ +1518,5 @@ > + Preferences::AddIntVarCache(&(prefs->nice[priority]), str.get()); > + > + // Read the oom_adj scores > + str = nsPrintfCString("hal.processPriorityManager.gonk.%s.OomScoreAdjust", > + processPriorityStr); And create another nsPrintfCString (named something other than str) here? @@ +1519,5 @@ > + > + // Read the oom_adj scores > + str = nsPrintfCString("hal.processPriorityManager.gonk.%s.OomScoreAdjust", > + processPriorityStr); > + Preferences::AddIntVarCache(&(prefs->oomScoreAdj[priority]), str.get()); nit: parens are unnecessary here. @@ +1522,5 @@ > + processPriorityStr); > + Preferences::AddIntVarCache(&(prefs->oomScoreAdj[priority]), str.get()); > + } > + > + Preferences::AddIntVarCache(&(prefs->lowPriorityNice), nit: parens are unnecessary here. @@ +1646,5 @@ > + */ > +struct ThreadPriorityPrefs { > + bool initialized; > + int nice[NUM_THREAD_PRIORITY]; > + int realTime[NUM_THREAD_PRIORITY]; nit: same comment about structs here. @@ +1655,5 @@ > + * watchers so that if they're dynamically changed the change is reflected on > + * the appropriate variables. > + */ > +void > +EnsureThreadPriorityPrefs(ThreadPriorityPrefs *prefs) nit: move star left @@ +1665,5 @@ > + for (int i = THREAD_PRIORITY_COMPOSITOR; i < NUM_THREAD_PRIORITY; i++) { > + ThreadPriority priority = static_cast<ThreadPriority>(i); > + > + // Read the nice values > + const char *threadPriorityStr = ThreadPriorityToString(priority); nit: move star left @@ +1666,5 @@ > + ThreadPriority priority = static_cast<ThreadPriority>(i); > + > + // Read the nice values > + const char *threadPriorityStr = ThreadPriorityToString(priority); > + nsCString str = nsPrintfCString("hal.gonk.%s.nice", threadPriorityStr); nit: same comment about nsPrintfCString @@ +1667,5 @@ > + > + // Read the nice values > + const char *threadPriorityStr = ThreadPriorityToString(priority); > + nsCString str = nsPrintfCString("hal.gonk.%s.nice", threadPriorityStr); > + Preferences::AddIntVarCache(&(prefs->nice[priority]), str.get()); nit: same comment about extra parens @@ +1671,5 @@ > + Preferences::AddIntVarCache(&(prefs->nice[priority]), str.get()); > + > + // Read the real-time priorities > + str = nsPrintfCString("hal.gonk.%s.rt_priority", threadPriorityStr); > + Preferences::AddIntVarCache(&(prefs->realTime[priority]), str.get()); nit: extra parens
Attachment #8494380 -
Flags: review?(dhylands) → review+
Assignee | ||
Comment 6•10 years ago
|
||
Thanks for the review Dave. I should have addressed all the points you raised though since the changes were numerous I'm asking for review again just in case. BTW I've rebased the patch on top of bug 1047277.
Attachment #8477674 -
Attachment is obsolete: true
Attachment #8494380 -
Attachment is obsolete: true
Attachment #8496024 -
Flags: review?(dhylands)
Comment 7•10 years ago
|
||
Comment on attachment 8496024 [details] [diff] [review] [PATCH v2] Cache the preferences affecting process and thread priorities The patch seems to be empty.
Attachment #8496024 -
Flags: review?(dhylands)
Assignee | ||
Comment 8•10 years ago
|
||
Ouch. Sorry, I had exported my mercurial tip without checking if it actually contained the patch or not and as it turned out, it didn't.
Attachment #8496024 -
Attachment is obsolete: true
Assignee | ||
Comment 9•10 years ago
|
||
Comment on attachment 8496774 [details] [diff] [review] [PATCH v2] Cache the preferences affecting process and thread priorities I finally had some time to check what this is buying us: I've measured the time it takes to run a SetProcessPriority() call using the monotonic clock and calculated the average and standard deviation over ~100 priority switches I triggered by hand in the same sequence. Without the patch this yielded 527µs ±274µs and with the patch it dropped to 431µs±178µs so this is buying us some measurable improvement. That being said I had no idea that fiddling with the files in /proc would be so expensive; the basic background-to-foreground transition burns ~1ms alone in SetProcessPriority(). Since I guess it's all the system calls we're doing that must be killing us I was wondering if we could devise a way to cut them down a bit.
Attachment #8496774 -
Flags: review?(dhylands)
Comment 10•10 years ago
|
||
Comment on attachment 8496774 [details] [diff] [review] [PATCH v2] Cache the preferences affecting process and thread priorities Review of attachment 8496774 [details] [diff] [review]: ----------------------------------------------------------------- Just one nit... ::: hal/gonk/GonkHal.cpp @@ +1676,5 @@ > + Preferences::AddIntVarCache(&prefs->priorities[i].nice, niceStr.get()); > + > + // Read the real-time priorities > + nsPrintfCString realTimeStr = nsPrintfCString("hal.gonk.%s.rt_priority", > + threadPriorityStr); nit: Use nsPrintfCString realTimeStr("hal.gonk.%s.rt_priority", threadPriorityStr); otherwise you're creating 2 nsPrintfCString's niceStr above was done properly.
Attachment #8496774 -
Flags: review?(dhylands) → review+
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Dave Hylands [:dhylands] from comment #10) > nit: Use > > nsPrintfCString realTimeStr("hal.gonk.%s.rt_priority", threadPriorityStr); > > otherwise you're creating 2 nsPrintfCString's > > niceStr above was done properly. Nice catch, I had missed it. Here's the updated patch, I'm carrying over the r=dhylands flag. The try run is here: https://tbpl.mozilla.org/?tree=Try&rev=3e489d86de25
Attachment #8499531 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8496774 -
Attachment is obsolete: true
Assignee | ||
Comment 12•10 years ago
|
||
Try is green save for known intermittents, time to land.
Keywords: checkin-needed
Comment 13•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b00ca413f85b
Keywords: checkin-needed
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b00ca413f85b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•