Hi Eliot,

My change of #prefEvent: is not about the temporary variable. It's about 
removing the loop which got duplicated. The original method looked like

 			...
 			method := anEvent item.
 			method pragmas do:[:pragma| | aPreference aPrefSymbol |
 				((pragma keyword == #preference:category:description:type:)
 					or: [pragma keyword == #preference:categoryList:description:type:]) ifTrue:[
 						aPrefSymbol := (aClass name,'>>', method selector) asSymbol.
 			...

Your version with #respondToPreferencePragmasInMethod:class: inlined would 
look like

 				...
 				method := anEvent item.
*				method pragmas do:
 					[:pragma|
*					method pragmas do:
 						[:pragma| | preference |
 						((pragma keyword beginsWith: #preference:)
 						 and: [self respondsTo: pragma keyword]) ifTrue:
 							[preference := self
 								perform: pragma keyword
 								withArguments: pragma arguments.
 				...

One of the "method pragmas do:" loops (marked with *) is superfluous. 
Since #respondToPreferencePragmasInMethod:class: contains the loop, and is 
also sent by other methods, I decided to remove the loop from #prefEvent:.

The #copy in #storePreferencesIn: was added in Squeak 4.1, or 4.2, because 
the dictionary could have been read and written concurrently. But this is 
not the case anymore.

Levente

On Wed, 1 Jul 2015, Eliot Miranda wrote:

> Hi Levente,
>    yes I like the repeat in atomicUpdatePreferences:.  That's safe.  I don't care much for the method temp in refEvent: cuz I think the message keyword types anEvent item adequately.  Not sure about storePreferencesIn: not taking a copy of preferencesDictionary. 
> Surely it's safer to take the copy.  So let me take the change to atomicUpdatePreferences: and get this show on the road :-).  Thanks for your review!
> 
> On Wed, Jul 1, 2015 at 4:30 AM, Levente Uzonyi <leves at elte.hu> wrote:
>       Hi Eliot,
>
>       I reviewed the code and made a few changes, which you can find in the Inbox as System-ul.748. Feel free to pick the ones you like.
>       The change of Preferences class >> #atomicUpdatePreferences: might seem unnecessary, because it's so unlikely to have multiple processes changing the preferences concurrently.
>
>       Levente
>
>       P.S.: Please note that I haven't tested the code.
>
>       On Tue, 30 Jun 2015, Eliot Miranda wrote:
>
>             Hi All, but especially Chris,
>                I just committed System-eem.745 to the inbox.  Please review.
>
>             As I say in the commit comment, this is step 1.  If the code looks OK, the next step is a version which moves the preferences dictionary into a class inst var, so that ServicePreferences sits happily below Preferences.  But for that I'll need
>             advice on how to write the
>             sequence of loads.  I *think* it's one configuration map and one package load.  The commit/configuration adds the class inst var and copies the DictionaryOfPreferences into it.  A subsequent commit replaces all methods that acess
>             DictionaryOfPreferences and
>             ServiceDictionaryOfPreferences with accesses to the class inst var
> 
>
>             Rewrite Preferences to eliminate the AccessProtect.
>             Use a copy, update copy, assign scheme to update
>             the preferences dictionary atomically.
>
>             Change Preferences access method compilation to
>             use Object>>#value to eliminate a block creation.
>
>             Change Preference initialization to eliminate the
>             isKindOf: Symbol.
>
>             This is step 1.  Given SystemPreferences it is clear
>             that the preferences dictionary should be stored in
>             a class inst var, so that SystemPreferences and
>             Preferences can share methods but access different
>             dictionaries.  The dictionaryOfProferences[:] accessors
>             are dubious as they break encapsulatiopn.  For example,
>             the reportPreferences: method, which is the only external
>             access, could insateaqd be moved into Preferences class.
>
>             On Tue, Jun 30, 2015 at 10:46 AM, Eliot Miranda <eliot.miranda at gmail.com> wrote:
>                   Hi Levente,  Hi Chris,
>
>                   On Tue, Apr 28, 2015 at 3:41 PM, Levente Uzonyi <leves at elte.hu> wrote:
>                         There's no need to store preferences in a data structure at all. We already have "pragma" preferences (since 4.1), which store the preference values independently. Since the 4.1 release it's a "permanent" goal to rewrite all
>             preferences to
>                         "pragma" preferences.
>                         We should just make it happen.
> 
>
>             This seems like a lot of work, and is work that can be done over time.  But right now we're suffering lock ups due to the Mutex in Preferences.  For example, the Notifier/Debugger accesses the scrollBarsOnRight preference and I've often seen
>             lock ups caused by
>             this.  So I propose that I fix the access to be as I described it.  There be no access lock except for adding/updating preferences.  So reading is done without synchronisation, and setting and/or adding is done by copying and assigning.  I also
>             propose to compile
>             preferences without creating a block, so
>
>             autoIndent
>             ^ self
>             valueOfFlag: #autoIndent
>             ifAbsent: true
>
>             instead of
>
>             autoIndent
>             ^ self
>             valueOfFlag: #autoIndent
>             ifAbsent: [true]
>
>             which is well-supported by both the Interpreter and the Cog VMs, given Object>>value ^self.  This to save space and time.
>
>                   Levente
>
>                   P.S.: Reverting that method will solve the concurrency issue.
>
>                   On Tue, 28 Apr 2015, Eliot Miranda wrote:
> 
> 
>
>                         On Tue, Apr 28, 2015 at 12:47 PM, Chris Muller <asqueaker at gmail.com> wrote:
>                               Wait, the newer one has a non-local return in it, but
>                               Mutex>>#critical: has an ensure: in it anyway, so maybe I don't see
>                               the problem..?
> 
>
>                         If one hits ctrl-period when the system is in the critical section then the debugger can't open because it interrupts the critical section, preventing the ensure block from running, attempts to access e.g.
>                         scroll bar preferences when it tries to open, and the system deadlocks.  So preferences either need to be *not* protected by a critical section, or the Debugger needs not to access preferences.
>
>                         IMO, we should try and write preferences so that they don't require a lock.  Writing them as a lock-free data structure would be a really good idea. First that critical section is slow and clunky.  Second, I
>                         presume it is there only for the rare case of a write to preferences, not to protect reads.
>
>                         IMO, a simple implementation which copied and replaced the entire preferences dictionary on write would be sufficient.  Sure there's a danger that some client would get a stale value if it read concurrently
>                         while there was a write, but then so what?  A preference is a preference, not a hard-and-fast value, and code should work accessing a preference no matter its value, so momentarily getting a stale value
>                         shouldn't matter.  So the implementation could be as simple as
>
>                         addPreference: aName categories: categoryList default: aValue balloonHelp: helpString projectLocal: localBoolean changeInformee: informeeSymbol changeSelector: aChangeSelector type: aType
>                         "Add or replace a preference as indicated.  Reuses the preexisting Preference object for this symbol, if there is one, so that UI artifacts that interact with it will remain valid."
>
>                         | aPreference aPrefSymbol |
>                         aPrefSymbol := aName asSymbol.
>                         aPreference := DictionaryOfPreferences
>                         at: aPrefSymbol
>                         ifAbsent:
>                         [| newPreference |
>                         newPreference := aPreference 
>                         name:aPrefSymbol
>                         defaultValue:aValue
>                         helpString:helpString
>                         localToProject:localBoolean
>                         categoryList:categoryList
>                         changeInformee:informeeSymbol
>                         changeSelector:aChangeSelector
>                         type: aType.
>                         AccessLock critical:
>                         [| newDict |
>                         newDict := DictionaryOfPreferences copy.
>                         newDict at: aPrefSymbol put: newPreference].
>                         self  compileAccessMethodForPreference:aPreference.
>                         newPreference]
> 
>
>                               On Tue, Apr 28, 2015 at 2:43 PM, Chris Muller <asqueaker at gmail.com> wrote:
>                               >> The above change restores the old behavior of locking up the image, so it
>                               >> should be reverted. An additional comment explaininng why aBlock must not be
>                               >> evaluated inside the argument of #accessDictionaryOfPreferencesIn: would be
>                               >> helpful.
>                               >
>                               > Ahh, because aBlock might have a non-local return in it, leaving the
>                               > Mutex unsignaled (and critical unenterable), is that right?
>                               >
>                               > Took me a minute to see that problem.
>                               >
>                               > Okay, I'll revert that method if no one else does by my next commit..
>                               >
>                               >> It would be even better to finally get rid of DictionaryOfPreferences.
>                               >>
>                               >>
>                               >> Levente
>                               >>
> 
> 
> 
>
>                         --
>                         best,Eliot
> 
> 
> 
> 
> 
> 
>
>             --
>             best,Eliot
> 
> 
> 
>
>             --
>             best,Eliot
> 
> 
> 
> 
> 
> 
> 
> --
> best,Eliot
> 
>