[squeak-dev] The Trunk: System-cmm.725.mcz
Levente Uzonyi
leves at elte.hu
Wed Jul 1 11:30:03 UTC 2015
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
>
>
More information about the Squeak-dev
mailing list
|