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