From 472643113fa9bac875eaeebecf5060a74e8670d9 Mon Sep 17 00:00:00 2001 From: StevenCostiou Date: Thu, 5 Dec 2019 14:22:44 +0100 Subject: [PATCH 1/6] Cache storing all nodes of a method associated to their bytecodes offsets. --- .../SindarinBytecodeToASTCacheTestd.class.st | 77 +++++++++++++++++++ Sindarin/SindarinBytecodeToASTCache.class.st | 74 ++++++++++++++++++ 2 files changed, 151 insertions(+) create mode 100644 Sindarin-Tests/SindarinBytecodeToASTCacheTestd.class.st create mode 100644 Sindarin/SindarinBytecodeToASTCache.class.st diff --git a/Sindarin-Tests/SindarinBytecodeToASTCacheTestd.class.st b/Sindarin-Tests/SindarinBytecodeToASTCacheTestd.class.st new file mode 100644 index 0000000..efb0146 --- /dev/null +++ b/Sindarin-Tests/SindarinBytecodeToASTCacheTestd.class.st @@ -0,0 +1,77 @@ +Class { + #name : #SindarinBytecodeToASTCacheTestd, + #superclass : #TestCase, + #instVars : [ + 'cache', + 'compiledMethod' + ], + #category : #'Sindarin-Tests' +} + +{ #category : #running } +SindarinBytecodeToASTCacheTestd >> setUp [ + "Hooks that subclasses may override to define the fixture of test." + super setUp. + compiledMethod := ScriptableDebuggerTests >> #helperMethod23. + cache := SindarinBytecodeToASTCache generateForCompiledMethod: compiledMethod +] + +{ #category : #helpers } +SindarinBytecodeToASTCacheTestd >> testCacheInInterval: interval equalsNode: aNode [ + interval do: [ :i | + self assert: (cache nodeForPC: i) identicalTo: aNode ] +] + +{ #category : #tests } +SindarinBytecodeToASTCacheTestd >> testCachedMethodNode [ + self assert: cache methodNode identicalTo: compiledMethod ast +] + +{ #category : #tests } +SindarinBytecodeToASTCacheTestd >> testFirstBCOffsetTest [ + self assert: cache firstBcOffset equals: compiledMethod initialPC +] + +{ #category : #tests } +SindarinBytecodeToASTCacheTestd >> testHigherThanLastBCOffsetAccessTest [ + | pc | + pc := cache lastBcOffset + 5. + self + assert: (cache nodeForPC: pc) + identicalTo: (compiledMethod sourceNodeForPC: pc) +] + +{ #category : #tests } +SindarinBytecodeToASTCacheTestd >> testLastBCOffsetTest [ + self + assert: cache lastBcOffset + equals: + compiledMethod ast ir startSequence withAllSuccessors last last + bytecodeOffset +] + +{ #category : #tests } +SindarinBytecodeToASTCacheTestd >> testLowerThanFirstBCOffsetAccessTest [ + self + testCacheInInterval: (0 to: cache firstBcOffset - 1) + equalsNode: compiledMethod ast +] + +{ #category : #tests } +SindarinBytecodeToASTCacheTestd >> testNodeForBCOffsetRangeTest [ + | pcRange | + pcRange := 47 to: 48. + pcRange do: [ :pc | + self + testCacheInInterval: pcRange + equalsNode: (compiledMethod sourceNodeForPC: pc) ] +] + +{ #category : #tests } +SindarinBytecodeToASTCacheTestd >> testNodeForBCOffsetTest [ + | pc | + pc := 51. + self + assert: (cache nodeForPC: pc) + identicalTo: (compiledMethod sourceNodeForPC: pc) +] diff --git a/Sindarin/SindarinBytecodeToASTCache.class.st b/Sindarin/SindarinBytecodeToASTCache.class.st new file mode 100644 index 0000000..04b3116 --- /dev/null +++ b/Sindarin/SindarinBytecodeToASTCache.class.st @@ -0,0 +1,74 @@ +" +I cache a mapping between possible bytecode offsets and the AST nodes they correspond to for a given compiled method. + +Instanciate me using my class method generateForCompiledMethod: and give me as parameter a compiled method. + +Use me through the node access API method nodeForPC: and give me a program counter as parameter. + +I store: +- firstBcOffset: The first bytecode pc. If you try to access a pc below that first pc, I return the method node. +- lastBcOffset: The last bytecode pc. If you try to access a pc after this last pc, I return the node associated with the last pc. +- bcToASTMap: A map associating each possible pc between firstBcOffset and lastBcOffset and the corresponding ast node. +- the methode node. +" +Class { + #name : #SindarinBytecodeToASTCache, + #superclass : #Object, + #instVars : [ + 'firstBcOffset', + 'lastBcOffset', + 'bcToASTMap', + 'methodNode' + ], + #category : #Sindarin +} + +{ #category : #initialization } +SindarinBytecodeToASTCache class >> generateForCompiledMethod: compiledMethod [ + ^self new generateForCompiledMethod: compiledMethod +] + +{ #category : #accessing } +SindarinBytecodeToASTCache >> bcToASTMap [ + ^ bcToASTMap +] + +{ #category : #accessing } +SindarinBytecodeToASTCache >> firstBcOffset [ + ^ firstBcOffset +] + +{ #category : #initialization } +SindarinBytecodeToASTCache >> generateForCompiledMethod: compiledMethod [ + | methodIR currentBcOffset | + methodNode := compiledMethod ast. + methodIR := methodNode ir. + bcToASTMap := Dictionary new. + firstBcOffset := methodIR startSequence withAllSuccessors first first + bytecodeOffset. + currentBcOffset := firstBcOffset. + methodIR startSequence withAllSuccessors do: [ :seq | + seq do: [ :ir | + ir ifNotNil: [ + currentBcOffset to: ir bytecodeOffset do: [ :i | + bcToASTMap at: i put: ir sourceNode ]. + currentBcOffset := ir bytecodeOffset + 1 ] ] ]. + lastBcOffset := currentBcOffset - 1 +] + +{ #category : #accessing } +SindarinBytecodeToASTCache >> lastBcOffset [ + ^ lastBcOffset +] + +{ #category : #accessing } +SindarinBytecodeToASTCache >> methodNode [ + ^ methodNode +] + +{ #category : #'node access' } +SindarinBytecodeToASTCache >> nodeForPC: pc [ + pc < firstBcOffset ifTrue: [ ^ methodNode ]. + pc > lastBcOffset ifTrue: [ ^ bcToASTMap at: lastBcOffset ]. + ^ bcToASTMap at: pc +] From a2605caaefc38f792df1e77db37b5c023ce87ec2 Mon Sep 17 00:00:00 2001 From: StevenCostiou Date: Thu, 5 Dec 2019 14:23:35 +0100 Subject: [PATCH 2/6] Renaming class --- ...> SindarinBytecodeToASTCacheTest.class.st} | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) rename Sindarin-Tests/{SindarinBytecodeToASTCacheTestd.class.st => SindarinBytecodeToASTCacheTest.class.st} (69%) diff --git a/Sindarin-Tests/SindarinBytecodeToASTCacheTestd.class.st b/Sindarin-Tests/SindarinBytecodeToASTCacheTest.class.st similarity index 69% rename from Sindarin-Tests/SindarinBytecodeToASTCacheTestd.class.st rename to Sindarin-Tests/SindarinBytecodeToASTCacheTest.class.st index efb0146..107f36f 100644 --- a/Sindarin-Tests/SindarinBytecodeToASTCacheTestd.class.st +++ b/Sindarin-Tests/SindarinBytecodeToASTCacheTest.class.st @@ -1,5 +1,5 @@ Class { - #name : #SindarinBytecodeToASTCacheTestd, + #name : #SindarinBytecodeToASTCacheTest, #superclass : #TestCase, #instVars : [ 'cache', @@ -9,7 +9,7 @@ Class { } { #category : #running } -SindarinBytecodeToASTCacheTestd >> setUp [ +SindarinBytecodeToASTCacheTest >> setUp [ "Hooks that subclasses may override to define the fixture of test." super setUp. compiledMethod := ScriptableDebuggerTests >> #helperMethod23. @@ -17,23 +17,23 @@ SindarinBytecodeToASTCacheTestd >> setUp [ ] { #category : #helpers } -SindarinBytecodeToASTCacheTestd >> testCacheInInterval: interval equalsNode: aNode [ +SindarinBytecodeToASTCacheTest >> testCacheInInterval: interval equalsNode: aNode [ interval do: [ :i | self assert: (cache nodeForPC: i) identicalTo: aNode ] ] { #category : #tests } -SindarinBytecodeToASTCacheTestd >> testCachedMethodNode [ +SindarinBytecodeToASTCacheTest >> testCachedMethodNode [ self assert: cache methodNode identicalTo: compiledMethod ast ] { #category : #tests } -SindarinBytecodeToASTCacheTestd >> testFirstBCOffsetTest [ +SindarinBytecodeToASTCacheTest >> testFirstBCOffsetTest [ self assert: cache firstBcOffset equals: compiledMethod initialPC ] { #category : #tests } -SindarinBytecodeToASTCacheTestd >> testHigherThanLastBCOffsetAccessTest [ +SindarinBytecodeToASTCacheTest >> testHigherThanLastBCOffsetAccessTest [ | pc | pc := cache lastBcOffset + 5. self @@ -42,7 +42,7 @@ SindarinBytecodeToASTCacheTestd >> testHigherThanLastBCOffsetAccessTest [ ] { #category : #tests } -SindarinBytecodeToASTCacheTestd >> testLastBCOffsetTest [ +SindarinBytecodeToASTCacheTest >> testLastBCOffsetTest [ self assert: cache lastBcOffset equals: @@ -51,14 +51,14 @@ SindarinBytecodeToASTCacheTestd >> testLastBCOffsetTest [ ] { #category : #tests } -SindarinBytecodeToASTCacheTestd >> testLowerThanFirstBCOffsetAccessTest [ +SindarinBytecodeToASTCacheTest >> testLowerThanFirstBCOffsetAccessTest [ self testCacheInInterval: (0 to: cache firstBcOffset - 1) equalsNode: compiledMethod ast ] { #category : #tests } -SindarinBytecodeToASTCacheTestd >> testNodeForBCOffsetRangeTest [ +SindarinBytecodeToASTCacheTest >> testNodeForBCOffsetRangeTest [ | pcRange | pcRange := 47 to: 48. pcRange do: [ :pc | @@ -68,7 +68,7 @@ SindarinBytecodeToASTCacheTestd >> testNodeForBCOffsetRangeTest [ ] { #category : #tests } -SindarinBytecodeToASTCacheTestd >> testNodeForBCOffsetTest [ +SindarinBytecodeToASTCacheTest >> testNodeForBCOffsetTest [ | pc | pc := 51. self From 81aeb38d0181f488b6b2d906cd90dddc0b3a76be Mon Sep 17 00:00:00 2001 From: StevenCostiou Date: Thu, 5 Dec 2019 14:24:39 +0100 Subject: [PATCH 3/6] Comment in one tricky test --- Sindarin-Tests/SindarinBytecodeToASTCacheTest.class.st | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Sindarin-Tests/SindarinBytecodeToASTCacheTest.class.st b/Sindarin-Tests/SindarinBytecodeToASTCacheTest.class.st index 107f36f..0ff572f 100644 --- a/Sindarin-Tests/SindarinBytecodeToASTCacheTest.class.st +++ b/Sindarin-Tests/SindarinBytecodeToASTCacheTest.class.st @@ -59,6 +59,8 @@ SindarinBytecodeToASTCacheTest >> testLowerThanFirstBCOffsetAccessTest [ { #category : #tests } SindarinBytecodeToASTCacheTest >> testNodeForBCOffsetRangeTest [ + "As we associate each node to each possible bytecode offset that can refer to it, + we have to check that associations are consistent between a range and a node" | pcRange | pcRange := 47 to: 48. pcRange do: [ :pc | From ced4ee91845f9080809d8ce449a0d2b02bf9835b Mon Sep 17 00:00:00 2001 From: StevenCostiou Date: Thu, 5 Dec 2019 15:49:50 +0100 Subject: [PATCH 4/6] Fixing missing mapping when different bytecode offsets map to the same AST node. --- .../SindarinBytecodeToASTCacheTest.class.st | 9 ++++---- Sindarin/SindarinBytecodeToASTCache.class.st | 22 ++++++++++++++----- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/Sindarin-Tests/SindarinBytecodeToASTCacheTest.class.st b/Sindarin-Tests/SindarinBytecodeToASTCacheTest.class.st index 0ff572f..94298bc 100644 --- a/Sindarin-Tests/SindarinBytecodeToASTCacheTest.class.st +++ b/Sindarin-Tests/SindarinBytecodeToASTCacheTest.class.st @@ -12,7 +12,7 @@ Class { SindarinBytecodeToASTCacheTest >> setUp [ "Hooks that subclasses may override to define the fixture of test." super setUp. - compiledMethod := ScriptableDebuggerTests >> #helperMethod23. + compiledMethod := ScriptableDebuggerTests >> #helperMethod12. cache := SindarinBytecodeToASTCache generateForCompiledMethod: compiledMethod ] @@ -61,12 +61,13 @@ SindarinBytecodeToASTCacheTest >> testLowerThanFirstBCOffsetAccessTest [ SindarinBytecodeToASTCacheTest >> testNodeForBCOffsetRangeTest [ "As we associate each node to each possible bytecode offset that can refer to it, we have to check that associations are consistent between a range and a node" + | pcRange | - pcRange := 47 to: 48. + pcRange := 0 to: cache lastBcOffset. pcRange do: [ :pc | self - testCacheInInterval: pcRange - equalsNode: (compiledMethod sourceNodeForPC: pc) ] + assert: (cache nodeForPC: pc) + identicalTo: (compiledMethod sourceNodeForPC: pc) ] ] { #category : #tests } diff --git a/Sindarin/SindarinBytecodeToASTCache.class.st b/Sindarin/SindarinBytecodeToASTCache.class.st index 04b3116..d8bff03 100644 --- a/Sindarin/SindarinBytecodeToASTCache.class.st +++ b/Sindarin/SindarinBytecodeToASTCache.class.st @@ -33,6 +33,19 @@ SindarinBytecodeToASTCache >> bcToASTMap [ ^ bcToASTMap ] +{ #category : #private } +SindarinBytecodeToASTCache >> fillMissingBCOffsetsWithLastBCOffsetNodes [ + | sortedBCOffsets | + sortedBCOffsets := bcToASTMap keys asSortedCollection. + 1 to: sortedBCOffsets size - 1 do: [ :index | + | bcAtIndex bcAtNextIndex | + bcAtIndex := sortedBCOffsets at: index. + bcAtNextIndex := sortedBCOffsets at: index + 1. + bcAtIndex < bcAtNextIndex ifTrue: [ + bcAtIndex to: bcAtNextIndex - 1 do: [ :i | + bcToASTMap at: i put: (bcToASTMap at: bcAtIndex) ] ] ] +] + { #category : #accessing } SindarinBytecodeToASTCache >> firstBcOffset [ ^ firstBcOffset @@ -44,16 +57,15 @@ SindarinBytecodeToASTCache >> generateForCompiledMethod: compiledMethod [ methodNode := compiledMethod ast. methodIR := methodNode ir. bcToASTMap := Dictionary new. - firstBcOffset := methodIR startSequence withAllSuccessors first first - bytecodeOffset. + firstBcOffset := compiledMethod initialPC. currentBcOffset := firstBcOffset. methodIR startSequence withAllSuccessors do: [ :seq | seq do: [ :ir | ir ifNotNil: [ - currentBcOffset to: ir bytecodeOffset do: [ :i | - bcToASTMap at: i put: ir sourceNode ]. + bcToASTMap at: ir bytecodeOffset ifAbsentPut: [ ir sourceNode ]. currentBcOffset := ir bytecodeOffset + 1 ] ] ]. - lastBcOffset := currentBcOffset - 1 + lastBcOffset := currentBcOffset - 1. + self fillMissingBCOffsetsWithLastBCOffsetNodes ] { #category : #accessing } From 72f3a8b939439ea6d8f001bee78ca73256566367 Mon Sep 17 00:00:00 2001 From: StevenCostiou Date: Thu, 5 Dec 2019 15:53:12 +0100 Subject: [PATCH 5/6] Added comment for complex method behavior --- Sindarin/SindarinBytecodeToASTCache.class.st | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Sindarin/SindarinBytecodeToASTCache.class.st b/Sindarin/SindarinBytecodeToASTCache.class.st index d8bff03..6f5cd72 100644 --- a/Sindarin/SindarinBytecodeToASTCache.class.st +++ b/Sindarin/SindarinBytecodeToASTCache.class.st @@ -35,6 +35,11 @@ SindarinBytecodeToASTCache >> bcToASTMap [ { #category : #private } SindarinBytecodeToASTCache >> fillMissingBCOffsetsWithLastBCOffsetNodes [ + "It happens that different bytecode offsets map to the same AST node. + These cases are detected when there is no node mapped between, for example, bcOffset 46 and bcOffset 50. + In that case, we take every possible bytecode index between 46 and 50 (i.e., 47, 48, 49), + and we map them to the same node as the last mapped bytecode offset, here 46." + | sortedBCOffsets | sortedBCOffsets := bcToASTMap keys asSortedCollection. 1 to: sortedBCOffsets size - 1 do: [ :index | From 98751b5dc3a288a1371ebc3df56cb2217d4e297b Mon Sep 17 00:00:00 2001 From: StevenCostiou Date: Thu, 5 Dec 2019 16:05:35 +0100 Subject: [PATCH 6/6] Changed node access by an optimization that lazily builds a cache mapping all possible bytecode offsets to their associated AST node. Instead of performing a search of the AST into the IR, it just builds the cache once and then only access the cache. On simple benchmarking tests, it speeds-up the execution by 4 to 5 times. --- Sindarin/SindarinDebugger.class.st | 32 +++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/Sindarin/SindarinDebugger.class.st b/Sindarin/SindarinDebugger.class.st index 1523b49..1f62f66 100644 --- a/Sindarin/SindarinDebugger.class.st +++ b/Sindarin/SindarinDebugger.class.st @@ -20,7 +20,9 @@ Class { #instVars : [ 'process', 'debugSession', - 'stepHooks' + 'stepHooks', + 'nodeMapForMethod', + 'debugStarted' ], #category : #Sindarin } @@ -96,7 +98,7 @@ SindarinDebugger >> contextIsAboutToSignalException: aContext [ "Returns whether aContext is about to execute a message-send of selector #signal to an instance of the Exception class (or one of its subclasses)" | node | - node := aContext method sourceNodeForPC: aContext pc. + node := (self nodeMapForMethod: aContext method) nodeForPC: aContext pc. node isMessage ifFalse: [ ^ false ]. node selector = #signal @@ -165,6 +167,7 @@ SindarinDebugger >> debug: aBlock [ debugSession deactivateEventTriggering. [ self selector = #newProcess] whileFalse: [ self step]. "Step the process to get out of the on:do: context added at the bottom of its stack" [self selector = #newProcess] whileTrue: [ self step ]. "Step the process so that it leaves BlockClosure>>#newProcess and enters the block for which a process was created" + debugStarted := true. ^ self ] @@ -177,6 +180,7 @@ SindarinDebugger >> debugSession [ { #category : #initialization } SindarinDebugger >> initialize [ stepHooks := OrderedCollection new. + debugStarted := false ] { #category : #stackAccess } @@ -248,7 +252,29 @@ SindarinDebugger >> method [ SindarinDebugger >> node [ "Returns the AST node about to be executed by the top context of the execution" - ^ self context method sourceNodeForPC: self context pc + debugStarted ifFalse: [ + "Until the debug session is started, node is returned using the unoptimized version" + "Sindarin starts by executing controlled steps to exit the caller code (e.g. tests) + before entering the actual debugged code, and we do not want to cache nodes for the + caller code." + ^ self context method sourceNodeForPC: self context pc ]. + "Once the hand is given to the tool or user, we start caching nodes for better performance" + ^ self nodeMap nodeForPC: self context pc +] + +{ #category : #astAndAstMapping } +SindarinDebugger >> nodeMap [ + ^self nodeMapForMethod: self context method +] + +{ #category : #astAndAstMapping } +SindarinDebugger >> nodeMapForMethod: aCompiledMethod [ + nodeMapForMethod ifNil: [ nodeMapForMethod := Dictionary new ]. + ^ (nodeMapForMethod + at: aCompiledMethod methodClass name + ifAbsentPut: [ Dictionary new ]) + at: aCompiledMethod selector + ifAbsentPut: [ SindarinBytecodeToASTCache generateForCompiledMethod: aCompiledMethod ] ] { #category : #'graphical debugger' }