The Trunk: System-cmm.725.mcz

Hi Levente,  Hi Chris,

On Tue, Apr 28, 2015 at 3:41 PM, Levente Uzonyi <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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

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 <[hidden email]> wrote:
      Hi Levente,  Hi Chris,

      On Tue, Apr 28, 2015 at 3:41 PM, Levente Uzonyi <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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