Issue 3340 in pharo: ThreadSafe DebuggerMethodMap

Status: Accepted
Owner: stephane.ducasse
Labels: Milestone-1.3 Type-Squeak

New issue 3340 by stephane.ducasse: ThreadSafe DebuggerMethodMap
http://code.google.com/p/pharo/issues/detail?id=3340

We should have a look at that fix from squeak

A new version of Tools was added to project The Inbox:
http://source.squeak.org/inbox/Tools-ul.278.mcz

==================== Summary ====================

Name: Tools-ul.278
Author: ul
Time: 15 November 2010, 8:46:10.672 am
UUID: d03d7286-1db1-f943-9e1c-6e86626b5f89
Ancestors: Tools-ul.277

Thread-safe DebuggerMethodMap

=============== Diff against Tools-ul.277 ===============

Item was changed:
  Object subclass: #DebuggerMethodMap
        instanceVariableNames: 'timestamp methodReference methodNode  
abstractSourceRanges sortedSourceMap'
+       classVariableNames: 'AccessLock MapCache MapCacheEntries'
-       classVariableNames: 'MapCache MapCacheEntries'
        poolDictionaries: ''
        category: 'Tools-Debugger'!

  !DebuggerMethodMap commentStamp: '<historical>' prior: 0!
  I am a place-holder for information needed by the Debugger to inspect  
method activations.  I insulate the debugger from details of code  
generation such as exact bytecode offsets and temporary variable  
locations.  I have two concreate subclasses, one for methods compiled using  
BlueBook blocks and one for methods compiled using Closures.  These classes  
deal with temporary variable access. My function is to abstract the source  
map away from actual bytecode pcs to abstract bytecode pcs.

  To reduce compilation time I try and defer as much computation to access  
time as possible as instances of me will be created after each compilation.

  I maintain a WeakIdentityDictionary of method to DebuggerMethodMap to  
cache maps.  I refer to my method through a WeakArray to keep the map cache  
functional. If the reference from a DebuggerMethodMap to its method were  
strong then the method would never be dropped from the cache because the  
reference from its map would keep it alive.!

Item was changed:
  ----- Method: DebuggerMethodMap class>>cacheDebugMap:forMethod: (in  
category 'debugger support') -----
  cacheDebugMap: aDebuggerMethodMap forMethod: aCompiledMethod
+
+       ^self protected: [
+               MapCache size >= MapCacheEntries ifTrue: [
+                       MapCache slowSize >= MapCacheEntries
+                               ifFalse: [ MapCache rehash ]
+                               ifTrue: [
+                                       | mapsByAge |
+                                       mapsByAge := MapCache keys sort:  
[ :m1 :m2 |
+                                               "We are holding strongly on  
the keys, so #at: is suitable."
+                                               (MapCache at: m1) timestamp  
< (MapCache at: m2) timestamp].
+                                       mapsByAge from: 1 to: mapsByAge  
size - MapCacheEntries do: [ :each |
+                                               MapCache removeKey: each ]  
] ].
+               MapCache
+                       at: aCompiledMethod
+                       put: aDebuggerMethodMap ]!
-       MapCache finalizeValues.
-       [MapCache size >= MapCacheEntries] whileTrue:
-               [| mapsByAge |
-                mapsByAge := MapCache keys asArray sort:
-                                                       [:m1 :m2|
-                                                       (MapCache at: m1)  
timestamp
-                                                       < (MapCache at: m2)  
timestamp].
-               mapsByAge notEmpty ifTrue: "There be race conditions and  
reentrancy issues here"
-                       [MapCache removeKey: mapsByAge last]].
-
-       ^MapCache
-               at: aCompiledMethod
-               put: aDebuggerMethodMap!

Item was changed:
  ----- Method: DebuggerMethodMap class>>forMethod: (in category 'instance  
creation') -----
  forMethod: aMethod "<CompiledMethod>"
        "Answer a DebuggerMethodMap suitable for debugging activations of  
aMethod.
         Answer an existing instance from the cache if it exists, cacheing a  
new one if required."
+
+       ^self protected: [
+               MapCache
+                       at: aMethod
+                       ifAbsent: [self
+                                               cacheDebugMap:
+                                                       (self
+                                                               forMethod:  
aMethod
+                                                               methodNode:  
aMethod methodNode)
+                                               forMethod: aMethod] ]!
-       ^MapCache
-               at: aMethod
-               ifAbsent: [self
-                                       cacheDebugMap:
-                                               (self
-                                                       forMethod: aMethod
-                                                       methodNode: aMethod  
methodNode)
-                                       forMethod: aMethod]!

Item was added:
+ ----- Method: DebuggerMethodMap class>>protected: (in  
category 'synchronization') -----
+ protected: aBlock
+
+       ^(AccessLock ifNil: [ AccessLock := Mutex new ]) critical: aBlock!

Item was changed:
  ----- Method: DebuggerMethodMap class>>voidMapCache (in category 'class  
initialization') -----
  voidMapCache
+
+       self protected: [
+               MapCache := WeakIdentityKeyDictionary new.
+               MapCacheEntries := 16 ]!
-       MapCache := WeakIdentityKeyDictionary new.
-       MapCacheEntries := 16!

Updates:
        Labels: -Milestone-1.3

Comment #1 on issue 3340 by [hidden email]: ThreadSafe  
DebuggerMethodMap
http://code.google.com/p/pharo/issues/detail?id=3340

(No comment was entered for this change.)