<div dir="ltr">Hi All, but especially Chris,<div><br></div><div>   I just committed System-eem.745 to the inbox.  Please review.</div><div><br></div><div>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&#39;ll need advice on how to write the sequence of loads.  I *think* it&#39;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</div><div><br></div><div><br></div><div>Rewrite Preferences to eliminate the AccessProtect.</div><div>Use a copy, update copy, assign scheme to update</div><div>the preferences dictionary atomically.</div><div><br></div><div>Change Preferences access method compilation to</div><div>use Object&gt;&gt;#value to eliminate a block creation.</div><div><br></div><div>Change Preference initialization to eliminate the</div><div>isKindOf: Symbol.</div><div><br></div><div>This is step 1.  Given SystemPreferences it is clear</div><div>that the preferences dictionary should be stored in</div><div>a class inst var, so that SystemPreferences and</div><div>Preferences can share methods but access different</div><div>dictionaries.  The dictionaryOfProferences[:] accessors</div><div>are dubious as they break encapsulatiopn.  For example,</div><div>the reportPreferences: method, which is the only external</div><div>access, could insateaqd be moved into Preferences class.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jun 30, 2015 at 10:46 AM, Eliot Miranda <span dir="ltr">&lt;<a href="mailto:eliot.miranda@gmail.com" target="_blank">eliot.miranda@gmail.com</a>&gt;</span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi Levente,  Hi Chris,<br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Tue, Apr 28, 2015 at 3:41 PM, Levente Uzonyi <span dir="ltr">&lt;<a href="mailto:leves@elte.hu" target="_blank">leves@elte.hu</a>&gt;</span> wrote:<br></span><span class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">There&#39;s no need to store preferences in a data structure at all. We already have &quot;pragma&quot; preferences (since 4.1), which store the preference values independently. Since the 4.1 release it&#39;s a &quot;permanent&quot; goal to rewrite all preferences to &quot;pragma&quot; preferences.<br>
We should just make it happen.</blockquote><div><br></div></span><div>This seems like a lot of work, and is work that can be done over time.  But right now we&#39;re suffering lock ups due to the Mutex in Preferences.  For example, the Notifier/Debugger accesses the scrollBarsOnRight preference and I&#39;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</div><div><br></div><div><div>autoIndent</div><div><span style="white-space:pre-wrap">        </span>^ self</div><div><span style="white-space:pre-wrap">                </span>valueOfFlag: #autoIndent</div><div><span style="white-space:pre-wrap">                </span>ifAbsent: true</div></div><div><br></div><div>instead of</div><div><br></div><div><div>autoIndent</div><div><span style="white-space:pre-wrap">        </span>^ self</div><div><span style="white-space:pre-wrap">                </span>valueOfFlag: #autoIndent</div><div><span style="white-space:pre-wrap">                </span>ifAbsent: [true]</div></div><div><br></div><div>which is well-supported by both the Interpreter and the Cog VMs, given Object&gt;&gt;value ^self.  This to save space and time.</div><div><div class="h5"><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span><font color="#888888">Levente<br>
</font></span><br>
P.S.: Reverting that method will solve the concurrency issue.<div><div><br>
<br>
On Tue, 28 Apr 2015, Eliot Miranda wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
On Tue, Apr 28, 2015 at 12:47 PM, Chris Muller &lt;<a href="mailto:asqueaker@gmail.com" target="_blank">asqueaker@gmail.com</a>&gt; wrote:<br>
      Wait, the newer one has a non-local return in it, but<br>
      Mutex&gt;&gt;#critical: has an ensure: in it anyway, so maybe I don&#39;t see<br>
      the problem..?<br>
<br>
<br>
If one hits ctrl-period when the system is in the critical section then the debugger can&#39;t open because it interrupts the critical section, preventing the ensure block from running, attempts to access e.g.<br>
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.<br>
<br>
IMO, we should try and write preferences so that they don&#39;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<br>
presume it is there only for the rare case of a write to preferences, not to protect reads.<br>
<br>
IMO, a simple implementation which copied and replaced the entire preferences dictionary on write would be sufficient.  Sure there&#39;s a danger that some client would get a stale value if it read concurrently<br>
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<br>
shouldn&#39;t matter.  So the implementation could be as simple as<br>
<br>
addPreference: aName categories: categoryList default: aValue balloonHelp: helpString projectLocal: localBoolean changeInformee: informeeSymbol changeSelector: aChangeSelector type: aType<br>
&quot;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.&quot;<br>
<br>
| aPreference aPrefSymbol |<br>
aPrefSymbol := aName asSymbol.<br>
aPreference := DictionaryOfPreferences<br>
at: aPrefSymbol<br>
ifAbsent:<br>
[| newPreference |<br>
newPreference := aPreference <br>
name:aPrefSymbol<br>
defaultValue:aValue<br>
helpString:helpString<br>
localToProject:localBoolean<br>
categoryList:categoryList<br>
changeInformee:informeeSymbol<br>
changeSelector:aChangeSelector<br>
type: aType.<br>
AccessLock critical:<br>
[| newDict |<br>
newDict := DictionaryOfPreferences copy.<br>
newDict at: aPrefSymbol put: newPreference].<br>
self  compileAccessMethodForPreference:aPreference.<br>
newPreference]<br>
<br>
<br>
      On Tue, Apr 28, 2015 at 2:43 PM, Chris Muller &lt;<a href="mailto:asqueaker@gmail.com" target="_blank">asqueaker@gmail.com</a>&gt; wrote:<br>
      &gt;&gt; The above change restores the old behavior of locking up the image, so it<br>
      &gt;&gt; should be reverted. An additional comment explaininng why aBlock must not be<br>
      &gt;&gt; evaluated inside the argument of #accessDictionaryOfPreferencesIn: would be<br>
      &gt;&gt; helpful.<br>
      &gt;<br>
      &gt; Ahh, because aBlock might have a non-local return in it, leaving the<br>
      &gt; Mutex unsignaled (and critical unenterable), is that right?<br>
      &gt;<br>
      &gt; Took me a minute to see that problem.<br>
      &gt;<br>
      &gt; Okay, I&#39;ll revert that method if no one else does by my next commit..<br>
      &gt;<br>
      &gt;&gt; It would be even better to finally get rid of DictionaryOfPreferences.<br>
      &gt;&gt;<br>
      &gt;&gt;<br>
      &gt;&gt; Levente<br>
      &gt;&gt;<br>
<br>
<br>
<br>
<br>
--<br>
best,Eliot<br>
<br>
</blockquote>
</div></div><br><br>
<br></blockquote></div></div></div><span class="HOEnZb"><font color="#888888"><br><br clear="all"><div><br></div>-- <br><div>best,<div>Eliot</div></div>
</font></span></div></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature">best,<div>Eliot</div></div>
</div>