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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20150630/ba182a62/attachment.htm