_gst_invalidate_method_cache and _gst_reset_inline_caches

Good Evening,

I looked into the excessive calls to _gst_reset_inline_caches. They
are mostly due calls to _gst_invalidate_method_cache and are done in
case of:


1.) Creating a new call-in process (in interp.c). This looks wrong. How
  should a new process change the look-up of a method?

2.) Installing a new method.

3.) VMpr_Behavior_flushCache. E.g. in gst-browser when selecting a category
  the following is executed:

(ip 52)[] in MethodDictionary>>#at:put:
(ip 30)[] in Semaphore>>#critical:
(ip 6)<unwind> BlockClosure>>#ensure:
(ip 8)Semaphore>>#critical:
(ip 8)MethodDictionary>>#at:put:
(ip 10)MethodDictionary(LookupTable)>>#add:
(ip 12)[] in Dictionary>>#select:
(ip 8)[] in LookupTable>>#associationsDo:
(ip 32)MethodDictionary(LookupTable)>>#keysAndValuesDo:
(ip 8)MethodDictionary(LookupTable)>>#associationsDo:
(ip 20)MethodDictionary(Dictionary)>>#select:
(ip 28)GtkMethodWidget>>#category:
(ip 10)GtkMethodWidget>>#class:withCategory:
(ip 12)GtkClassBrowserWidget>>#updateInstanceSideMethodCategory:
(ip 14)CategoryState>>#updateBrowser:
(ip 8)GtkClassBrowserWidget>>#state:
(ip 6)[] in GtkClassBrowserWidget>>#updateState:
(ip 8)[] in GtkClassBrowserWidget>>#checkCodeWidgetAndUpdate:
(ip 24)GtkClassBrowserWidget>>#saveCodeOr:
(ip 8)GtkClassBrowserWidget>>#checkCodeWidgetAndUpdate:
(ip 14)GtkClassBrowserWidget>>#updateState:
(ip 12)GtkClassBrowserWidget>>#onInstanceSideCategoryChanged


Starting gst-browser with some instrumentation gives output like this:

WALKED 35333 ICs.. 35331 cold diff 2
WALKED 35333 ICs.. 35268 cold diff 65

...

showing that the caches are mostly flushed twice. When doing a

  3+3 CTRL-P

a.) WALKED 84729 ICs.. 84129 cold diff 600
b.) WALKED 84729 ICs.. 84727 cold diff 2
c.) WALKED 84729 ICs.. 84656 cold diff 73

a.)
(ip 52)[] in MethodDictionary>>#at:put:
(ip 30)[] in Semaphore>>#critical:

b.)
#1  0xb7f74625 in _gst_invalidate_method_cache () at interp.c:2363
#2  0xb7f1ddb9 in install_method (methodOOP=methodOOP@entry=0x40856308, classOOP=0x406306f0)
    at comp.c:2556

c.)
(ip 38)[] in MethodDictionary>>#removeKey:ifAbsent:
(ip 30)[] in Semaphore>>#critical:
(ip 6)<unwind> BlockClosure>>#ensure:
(ip 8)Semaphore>>#critical:
(ip 6)MethodDictionary>>#removeKey:ifAbsent:
(ip 34)Behavior>>#removeSelector:ifAbsent:
(ip 38)GtkWorkspaceWidget(GtkTextWidget)>>#doWithoutForkIt:



Sorry. A lot of facts now some judgement and proposals.


Removing of calls:
1.) I think we can safely remove the flush from the call-in process
creation. This code has been there since the initial import.

2.) I think we can remove the invalidate from install_method in case
the kernel is initialized. In that case we will use >>#at:put: to
add the method that will already end in a flushCache.



Optimizations:

 MethodDictionary>>#select: will create a new MethodDictionary that
 will flush the cache when it is populated. It is wasteful and we could
 avoid this. It will only benefit VisualGST.


 Most inline caches are in-active (okay creating a VM that has both the
 jit and interpreter in one is another topic). The ideas we have talked
 about so far are:

 * OpenMP to make the invalidate spread across different cores
 * Add a global version to the IC. So the fast path would have an
   additional load, compare, branch.
 * Create a list or a flat array (and if more than 1024 entries) and
   add active entries there. So this would create additional calls
   on the hot path as well.


comments?

        holger

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

Il 21/12/2013 20:22, Holger Hans Peter Freyther ha scritto:
> Removing of calls:
> 1.) I think we can safely remove the flush from the call-in process
> creation. This code has been there since the initial import.

It's possible that it is a relic dating back to before the introduction
of MethodDictionary.

> 2.) I think we can remove the invalidate from install_method in case
> the kernel is initialized. In that case we will use >>#at:put: to
> add the method that will already end in a flushCache.

Interesting.

>  MethodDictionary>>#select: will create a new MethodDictionary that
>  will flush the cache when it is populated. It is wasteful and we could
>  avoid this. It will only benefit VisualGST.

I think the species of MethodDictionary should be LookupTable?

Paolo


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

On Sat, Dec 21, 2013 at 09:38:26PM +0100, Paolo Bonzini wrote:

> I think the species of MethodDictionary should be LookupTable?

It is more complicated than this. E.g. with the >>#remove:


 remove: anAssociation [
        ...
           copy := self copy.
           result := copy dangerouslyRemove: anAssociation.
           self become: copy.
           Behavior flushCache].
        ...
    ]


So >>#copy should return a MethodDictionary. I considered just
changing >>#copyEmpty: but >>#shallowCopy is already using this.

What about introducing a >>#fastSelect: inside VisualGST to
avoid the flush? In the case of Doit/DoIt in VisualGST and Behavior..
what if we introduce a method that just creates a CompiledMethod
but does not add it to a method dictionary?

holger



Example issue while running the test code:

Swazoo.SwazooCompilerTest>>#testEvaluate did not understand #dangerouslyRemoveKey:
TestVerboseLog>>logError: (SUnit.star#VFS.ZipFile/SUnit.st:609)
Swazoo.SwazooCompilerTest(TestCase)>>logError: (SUnit.star#VFS.ZipFile/SUnit.st:877)
[] in TestResult>>runCase: (SUnit.star#VFS.ZipFile/SUnit.st:443)
MessageNotUnderstood(Exception)>>activateHandler: (ExcHandling.st:515)
MessageNotUnderstood(Exception)>>signal (ExcHandling.st:254)
LookupTable(Object)>>doesNotUnderstand: #dangerouslyRemoveKey: (SysExcept.st:1408)
[] in MethodDictionary>>removeKey:ifAbsent: (MethodDict.st:108)
[] in Semaphore>>critical: (Semaphore.st:80)
BlockClosure>>ensure: (BlkClosure.st:271)
Semaphore>>critical: (Semaphore.st:60)
MethodDictionary>>removeKey:ifAbsent: (MethodDict.st:111)
Metaclass(Behavior)>>removeSelector:ifAbsent: (Behavior.st:261)
Behavior class(Behavior)>>evalString:to: (Behavior.st:432)
Behavior class(Behavior)>>evaluate:to: (Behavior.st:503)
SpEnvironment class>>evaluate:receiver:in: (Sport.star#VFS.ZipFile/sport.st:385)


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

The code is already flushing the code on adding a new method _and_
MethodDictionary>>#at:put: and there should be noting in the process
creation that changes the MethodDictionary.

2013-12-22  Holger Hans Peter Freyther  <[hidden email]>

        * interp.c: Do not flush the cache when creating a new
        process.
---
 libgst/ChangeLog | 5 +++++
 libgst/interp.c  | 1 -
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/libgst/ChangeLog b/libgst/ChangeLog
index 1fb33be..03287b1 100644
--- a/libgst/ChangeLog
+++ b/libgst/ChangeLog
@@ -1,3 +1,8 @@
+2013-12-22  Holger Hans Peter Freyther  <[hidden email]>
+
+ * interp.c: Do not flush the cache when creating a new
+ process.
+
 2013-12-14  Holger Hans Peter Freyther  <[hidden email]>
 
  * cint.c: Fix the compilation on FreeBSD.
diff --git a/libgst/interp.c b/libgst/interp.c
index b30ebbd..0a01361 100644
--- a/libgst/interp.c
+++ b/libgst/interp.c
@@ -2143,7 +2143,6 @@ create_callin_process (OOP contextOOP)
   /* Put initialProcessOOP in the root set */
   add_first_link (initialProcessListOOP, initialProcessOOP);
 
-  _gst_invalidate_method_cache ();
   return (initialProcessOOP);
 }
 
--
1.8.5.1


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

install_method was flushing the cache twice. Once by calling
>>#at:put: on the MethodDictionary and once by install_method
itself. Only do that when the kernel is not initialized yet.

2013-12-22  Holger Hans Peter Freyther  <[hidden email]>

        * comp.c: Only flush the cache once when adding a new
        CompiledMethod.
---
 libgst/ChangeLog | 5 +++++
 libgst/comp.c    | 3 ++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/libgst/ChangeLog b/libgst/ChangeLog
index 03287b1..6e48372 100644
--- a/libgst/ChangeLog
+++ b/libgst/ChangeLog
@@ -1,5 +1,10 @@
 2013-12-22  Holger Hans Peter Freyther  <[hidden email]>
 
+ * comp.c: Only flush the cache once when adding a new
+ CompiledMethod.
+
+2013-12-22  Holger Hans Peter Freyther  <[hidden email]>
+
  * interp.c: Do not flush the cache when creating a new
  process.
 
diff --git a/libgst/comp.c b/libgst/comp.c
index 0efcae1..040994c 100644
--- a/libgst/comp.c
+++ b/libgst/comp.c
@@ -2553,7 +2553,8 @@ install_method (OOP methodOOP, OOP classOOP)
 #ifdef VERIFY_COMPILED_METHODS
   _gst_verify_sent_method (methodOOP);
 #endif
-  _gst_invalidate_method_cache ();
+  if (!_gst_kernel_initialized)
+    _gst_invalidate_method_cache ();
 }
 
 OOP
--
1.8.5.1


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

On 22/12/2013 10:48, Holger Hans Peter Freyther wrote: Why not doing:

--- a/kernel/MethodDictionary.st
+++ b/kernel/MethodDictionary.st
@@ -119,6 +119,32 @@ interpreter.'>
         self mutex critical: [ self growBy: 0 ]
      ]

+    select: aBlock [
+    "Answer a new dictionary containing the key/value pairs for which
aBlock
+     returns true. aBlock only receives the value part of the pairs."
+
+    <category: 'dictionary enumerating'>
+    | newDict |
+    newDict := self copyEmpty: self capacity.
+    self
+        associationsDo: [:assoc | (aBlock value: assoc value) ifTrue:
[newDict dangerouslyAt: assoc key put: assoc value]].
+    ^newDict
+    ]
+
+    dangerouslyAt: aKey put: aValue [
+    <category: 'private methods'>
+
+        | index |
+        index := self findIndex: aKey.
+        (self primAt: index) isNil ifTrue: [
+                                             self incrementTally
ifTrue: [ index := self findIndex: aKey ].
+                                             self primAt: index put: aKey
+                                           ]
+                                   ifFalse: [ (self valueAt: index)
discardTranslation ].
+        self valueAt: index put: aValue.
+       ^ aValue
+    ]
+


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

From: Gwenaël Casaccio <[hidden email]>

The default >>#select: implementation will call Behavior>>#flushCache:
for every CompiledMethod that is inserted into the new Dictionary.
Avoid this by creating a dedicated >>#select: implementation that avoid
the cache flush.

2014-01-19  Gwenael Casaccio  <[hidden email]>

        * kernel/MethodDict.st: Introduce >>#select: and
        >>#dangerouslyAt:put:.
---
 ChangeLog            |  5 +++++
 kernel/MethodDict.st | 31 ++++++++++++++++++++++++++++++-
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index ea47151..46a0bc5 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2014-01-19  Gwenael Casaccio  <[hidden email]>
+
+ * kernel/MethodDict.st: Introduce >>#select: and
+ >>#dangerouslyAt:put:.
+
 2013-12-14  Holger Hans Peter Freyther  <[hidden email]>
 
  * configure.ac: Check for environ with AC_CHECK_DECLS.
diff --git a/kernel/MethodDict.st b/kernel/MethodDict.st
index 155a8ac..cd02090 100644
--- a/kernel/MethodDict.st
+++ b/kernel/MethodDict.st
@@ -7,7 +7,7 @@
 
 "======================================================================
 |
-| Copyright 1999, 2000, 2001, 2002 Free Software Foundation, Inc.
+| Copyright 1999, 2000, 2001, 2002, 2014 Free Software Foundation, Inc.
 | Written by Paolo Bonzini.
 |
 | This file is part of the GNU Smalltalk class library.
@@ -119,6 +119,35 @@ interpreter.'>
        self mutex critical: [ self growBy: 0 ]
     ]
 
+    select: aBlock [
+        "Answer a new dictionary containing the key/value pairs for which aBlock
+        returns true. aBlock only receives the value part of the pairs."
+
+        <category: 'dictionary enumerating'>
+        | newDict |
+        newDict := self copyEmpty: self capacity.
+        self
+            associationsDo: [:assoc |
+                (aBlock value: assoc value)
+                    ifTrue: [newDict dangerouslyAt: assoc key put: assoc value]].
+        ^newDict
+    ]
+
+    dangerouslyAt: aKey put: aValue [
+        <category: 'private methods'>
+
+        | index |
+        index := self findIndex: aKey.
+        (self primAt: index) isNil
+            ifTrue: [
+                self incrementTally
+                    ifTrue: [ index := self findIndex: aKey ].
+                self primAt: index put: aKey]
+            ifFalse: [ (self valueAt: index) discardTranslation ].
+        self valueAt: index put: aValue.
+       ^ aValue
+    ]
+
     dangerouslyRemove: anAssociation [
  "This is not really dangerous.  But if normal removal
  were done WHILE a MethodDictionary were being used, the
--
1.8.5.2


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

On Sun, Jan 19, 2014 at 05:14:36PM +0100, Holger Hans Peter Freyther wrote:

Dear Gwenaël,

I would like to introduce the optimization and have one more question.

Why do we need the ifFalse: case? This would flush the JITed code of
a translated method but we have not modified that method? Can you please
elaborate?

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk