989020 - Write a helper to make it easy to access preferences off-Main Thread

Status: RESOLVED WONTFIX

(Core :: Preferences: Backend, defect)

Comment 1

[Benjamin Smedberg]

Could you be more specific? In general I think it should rarely if ever be necessary to access real preferences from other threads.

Comment 2

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: WIP - OffMainThreadPreferences, v1

How does this look for a first pass?

Comment 3

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
>
> Could you be more specific? In general I think it should rarely if ever be
> necessary to access real preferences from other threads.

For myself, I've been working on shim layers to improve test coverage of b2g's camera code, and right now the mochitests push test configurations through prefs, which get picked up on a worker thread in Gecko.

Bug bug 980419 in mind, I found myself bouncing nsRunnables off of the Main Thread to get prefs, and figured it would be easier if there were something to handle that for me, like the attached code.

Comment 4

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: WIP - OffMainThreadPreferences, v1

Attach the complete patch, this time.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

This will help me fix bug 980027 as well.  Thanks Mike!

Comment 6

[[:fabrice] Fabrice Desré]

Comment on attachment 8398192 [details] [diff] [review]
WIP - OffMainThreadPreferences, v1

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

Mike, I have no time to look at that today and will be PTO next week. Better find someone else...

Comment 7

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8398192 [details] [diff] [review]
WIP - OffMainThreadPreferences, v1

bent, what do you think?

Comment 8

[Benjamin Smedberg]

Comment on attachment 8398192 [details] [diff] [review]
WIP - OffMainThreadPreferences, v1

This is not a good pattern.

To the extent that we actually need to control non-mainthread code using prefs, we should record the pref values either at startup or when we launch the task in question.

Comment 9

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
>
> This is not a good pattern.
> 
> To the extent that we actually need to control non-mainthread code using
> prefs, we should record the pref values either at startup or when we launch
> the task in question.

What should be done in the case where this is not possible? Currently people either pull prefs off-Main Thread (see bug bug 980419) or they will code up their own repetitive solutions to bouncing events off of the Main Thread.

Comment 10

[Benjamin Smedberg]

I haven't seen any of those cases. I don't know what the threading model of GonkCameraManager or ICamera is (no docs or comments!?) but it seems that the main-thread factories for those objects should collect the necessary prefs, or in the worst case the content module constructor should archive the necessary values in atomic globals.

In no case does it make sense to bounce events around.

Comment 11

[Mike Habicher [:mikeh] (high bugzilla latency)]

The issue is the necessary prefs can't be collected all at once because they change over time as the automated tests are running.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8398192 [details] [diff] [review]
WIP - OffMainThreadPreferences, v1

In general there are two patterns that I've needed in workers:

  1. Grab prefs at startup (or the first time a worker is created) and stash them in a
     readonly spot (const global, etc).
  2. Install pref observers that monitor prefs that I expect to change and will need to
     respond to. These observers send messages to running workers so that they update
     themselves.

In general trying to do things magically will almost always have unintended consequences I think. Your patch uses NS_DISPATCH_SYNC for instance, and that will spin the event loop on the calling thread (assuming the calling thread is an nsThread and not something simpler like a chromium/NSPR/native thread... in which case I guess it'll just fail).

Comment 13

[Mike Habicher [:mikeh] (high bugzilla latency)]

smedberg, bent--thanks for the feedback. I'm going to close this bug and open a new one with a Camera-specific based on comment 12.