Levente, I object :) I find the findBinary:do:ifNone: extremely ugly and much prefer the findNearbyBinaryIndex: usage. Would you mind if we reverted to that?
On Thu, Sep 8, 2011 at 2:04 PM, commits@source.squeak.org wrote:
A new version of Tools was added to project The Inbox: http://source.squeak.org/inbox/Tools-ul.378.mcz
==================== Summary ====================
Name: Tools-ul.378 Author: ul Time: 8 September 2011, 10:57:10.107 pm UUID: 0262d60e-4ceb-b246-8d3f-96424d2aa1ce Ancestors: Tools-eem.377
Simplified DebuggerMethodMap >> #rangeForPC:contextIsActiveContext:.
- copy and sort the associations in a bit simpler and probably faster way
- removed unnecessary checks (#isNil after non-nil assignment)
- uses Array instead of SortedCollection :)
- uses Juan's general purpose binary search instead of a custom one (it
also handles all special cases)
- uses #detectMax: instead of a specialized #inject:into:
=============== Diff against Tools-eem.377 ===============
Item was changed: ----- Method: DebuggerMethodMap>>rangeForPC:contextIsActiveContext: (in category 'source mapping') ----- rangeForPC: contextsConcretePC contextIsActiveContext: contextIsActiveContext "Answer the indices in the source code for the supplied pc. If the context is the actve context (is at the hot end of the stack) then its pc is the current pc. But if the context isn't, because it is suspended sending a message, then its current pc is the previous pc."
| pc |
| pc i end | pc := self method abstractPCForConcretePC: (contextIsActiveContext ifTrue: [contextsConcretePC] ifFalse: [(self method pcPreviousTo:
contextsConcretePC)
ifNotNil: [:prevpc|
prevpc]
ifNil:
[contextsConcretePC]]). (self abstractSourceMap includesKey: pc) ifTrue: [^self abstractSourceMap at: pc].
sortedSourceMap ifNil: [
sortedSourceMap := self abstractSourceMap associations
replace: [ :each | each copy ];
sort ].
^sortedSourceMap
findBinary: [ :assoc | pc - assoc key ]
do: [ :item | item value ]
ifNone: [ :lower :upper |
lower
ifNil: [ 1 to: 0 ]
ifNotNil: [
upper
ifNotNil: [ upper value ]
"No match, but a nearby element."
ifNil: [
| end |
end :=
sortedSourceMap detectMax: [ :each | each value last ].
end + 1 to: end ] ]
]
sortedSourceMap ifNil:
[sortedSourceMap := self abstractSourceMap.
sortedSourceMap := (sortedSourceMap keys collect:
[:key| key
-> (sortedSourceMap at: key)]) asSortedCollection].
(sortedSourceMap isNil or: [sortedSourceMap isEmpty]) ifTrue: [^1
to: 0].
i := sortedSourceMap findNearbyBinaryIndex: [:assoc| pc - assoc
key].
i < 1 ifTrue: [^1 to: 0].
i > sortedSourceMap size ifTrue:
[end := sortedSourceMap inject: 0 into:
[:prev :this | prev max: this value last].
^end+1 to: end].
^(sortedSourceMap at: i) value "| method source scanner map | method := DebuggerMethodMap compiledMethodAt:
#rangeForPC:contextIsActiveContext:. source := method getSourceFromFile asString. scanner := InstructionStream on: method. map := method debuggerMap. Array streamContents: [:ranges| [scanner atEnd] whileFalse: [| range | range := map rangeForPC: scanner pc contextIsActiveContext: true. ((map abstractSourceMap includesKey: scanner abstractPC) and: [range first ~= 0]) ifTrue: [ranges nextPut: (source copyFrom: range first to: range last)]. scanner interpretNextInstructionFor: InstructionClient new]]"!
On Thu, 8 Sep 2011, Eliot Miranda wrote:
Levente, I object :) I find the findBinary:do:ifNone: extremely ugly and much prefer the findNearbyBinaryIndex: usage. Would you mind if we reverted to that?
I pushed it to the inbox, so there's nothing to revert. I prefer to have as few implementations of binary search as possible. Did you check the other changes? Are those acceptable?
Levente
P.S.: I just saw that Henrik wrote a mail about this method on the Pharo list with some other simplifications.
On Thu, Sep 8, 2011 at 2:04 PM, commits@source.squeak.org wrote:
A new version of Tools was added to project The Inbox: http://source.squeak.org/inbox/Tools-ul.378.mcz
==================== Summary ====================
Name: Tools-ul.378 Author: ul Time: 8 September 2011, 10:57:10.107 pm UUID: 0262d60e-4ceb-b246-8d3f-96424d2aa1ce Ancestors: Tools-eem.377
Simplified DebuggerMethodMap >> #rangeForPC:contextIsActiveContext:.
- copy and sort the associations in a bit simpler and probably faster way
- removed unnecessary checks (#isNil after non-nil assignment)
- uses Array instead of SortedCollection :)
- uses Juan's general purpose binary search instead of a custom one (it
also handles all special cases)
- uses #detectMax: instead of a specialized #inject:into:
=============== Diff against Tools-eem.377 ===============
Item was changed: ----- Method: DebuggerMethodMap>>rangeForPC:contextIsActiveContext: (in category 'source mapping') ----- rangeForPC: contextsConcretePC contextIsActiveContext: contextIsActiveContext "Answer the indices in the source code for the supplied pc. If the context is the actve context (is at the hot end of the stack) then its pc is the current pc. But if the context isn't, because it is suspended sending a message, then its current pc is the previous pc."
| pc |
| pc i end | pc := self method abstractPCForConcretePC: (contextIsActiveContext ifTrue: [contextsConcretePC] ifFalse: [(self method pcPreviousTo:
contextsConcretePC)
ifNotNil: [:prevpc|
prevpc]
ifNil:
[contextsConcretePC]]). (self abstractSourceMap includesKey: pc) ifTrue: [^self abstractSourceMap at: pc].
sortedSourceMap ifNil: [
sortedSourceMap := self abstractSourceMap associations
replace: [ :each | each copy ];
sort ].
^sortedSourceMap
findBinary: [ :assoc | pc - assoc key ]
do: [ :item | item value ]
ifNone: [ :lower :upper |
lower
ifNil: [ 1 to: 0 ]
ifNotNil: [
upper
ifNotNil: [ upper value ]
"No match, but a nearby element."
ifNil: [
| end |
end :=
sortedSourceMap detectMax: [ :each | each value last ].
end + 1 to: end ] ]
]
sortedSourceMap ifNil:
[sortedSourceMap := self abstractSourceMap.
sortedSourceMap := (sortedSourceMap keys collect:
[:key| key
-> (sortedSourceMap at: key)]) asSortedCollection].
(sortedSourceMap isNil or: [sortedSourceMap isEmpty]) ifTrue: [^1
to: 0].
i := sortedSourceMap findNearbyBinaryIndex: [:assoc| pc - assoc
key].
i < 1 ifTrue: [^1 to: 0].
i > sortedSourceMap size ifTrue:
[end := sortedSourceMap inject: 0 into:
[:prev :this | prev max: this value last].
^end+1 to: end].
^(sortedSourceMap at: i) value "| method source scanner map | method := DebuggerMethodMap compiledMethodAt:
#rangeForPC:contextIsActiveContext:. source := method getSourceFromFile asString. scanner := InstructionStream on: method. map := method debuggerMap. Array streamContents: [:ranges| [scanner atEnd] whileFalse: [| range | range := map rangeForPC: scanner pc contextIsActiveContext: true. ((map abstractSourceMap includesKey: scanner abstractPC) and: [range first ~= 0]) ifTrue: [ranges nextPut: (source copyFrom: range first to: range last)]. scanner interpretNextInstructionFor: InstructionClient new]]"!
-- best, Eliot
On Thu, Sep 8, 2011 at 2:18 PM, Levente Uzonyi leves@elte.hu wrote:
On Thu, 8 Sep 2011, Eliot Miranda wrote:
Levente, I object :) I find the findBinary:do:ifNone: extremely ugly and
much prefer the findNearbyBinaryIndex: usage. Would you mind if we reverted to that?
I pushed it to the inbox, so there's nothing to revert.
Ah, sorry. Saw the commit notice and assumed. Apologies.
I prefer to have as few implementations of binary search as possible.
Yes, but findNearbyBinaryIndex: pays its way. It saved 11 lines and is way more comprehensible.
Did you check the other changes? Are those acceptable?
I'm mulling them over :) I like a variable for the context pc (which Henrik didn't suggest) and I like using an at:ifAbsent: block. So I'll hopefully submit a new version sometime soon.
Levente
P.S.: I just saw that Henrik wrote a mail about this method on the Pharo list with some other simplifications.
On Thu, Sep 8, 2011 at 2:04 PM, commits@source.squeak.org wrote:
A new version of Tools was added to project The Inbox:
http://source.squeak.org/**inbox/Tools-ul.378.mczhttp://source.squeak.org/inbox/Tools-ul.378.mcz
==================== Summary ====================
Name: Tools-ul.378 Author: ul Time: 8 September 2011, 10:57:10.107 pm UUID: 0262d60e-4ceb-b246-8d3f-**96424d2aa1ce Ancestors: Tools-eem.377
Simplified DebuggerMethodMap >> #rangeForPC:**contextIsActiveContext:.
- copy and sort the associations in a bit simpler and probably faster way
- removed unnecessary checks (#isNil after non-nil assignment)
- uses Array instead of SortedCollection :)
- uses Juan's general purpose binary search instead of a custom one (it
also handles all special cases)
- uses #detectMax: instead of a specialized #inject:into:
=============== Diff against Tools-eem.377 ===============
Item was changed: ----- Method: DebuggerMethodMap>>rangeForPC:**contextIsActiveContext: (in category 'source mapping') ----- rangeForPC: contextsConcretePC contextIsActiveContext: contextIsActiveContext "Answer the indices in the source code for the supplied pc. If the context is the actve context (is at the hot end of the stack) then its pc is the current pc. But if the context isn't, because it is suspended sending a message, then its current pc is the previous pc."
| pc |
| pc i end | pc := self method abstractPCForConcretePC: (contextIsActiveContext ifTrue: [contextsConcretePC] ifFalse: [(self method pcPreviousTo:
contextsConcretePC)
ifNotNil: [:prevpc|
prevpc]
ifNil:
[contextsConcretePC]]). (self abstractSourceMap includesKey: pc) ifTrue: [^self abstractSourceMap at: pc].
sortedSourceMap ifNil: [
sortedSourceMap := self abstractSourceMap associations
replace: [ :each | each copy ];
sort ].
^sortedSourceMap
findBinary: [ :assoc | pc - assoc key ]
do: [ :item | item value ]
ifNone: [ :lower :upper |
lower
ifNil: [ 1 to: 0 ]
ifNotNil: [
upper
ifNotNil: [ upper value ]
"No match, but a nearby element."
ifNil: [
| end |
end :=
sortedSourceMap detectMax: [ :each | each value last ].
end + 1 to: end ]
] ]
sortedSourceMap ifNil:
[sortedSourceMap := self abstractSourceMap.
sortedSourceMap := (sortedSourceMap keys collect:
[:key|
key -> (sortedSourceMap at: key)]) asSortedCollection].
(sortedSourceMap isNil or: [sortedSourceMap isEmpty]) ifTrue: [^1
to: 0].
i := sortedSourceMap findNearbyBinaryIndex: [:assoc| pc - assoc
key].
i < 1 ifTrue: [^1 to: 0].
i > sortedSourceMap size ifTrue:
[end := sortedSourceMap inject: 0 into:
[:prev :this | prev max: this value last].
^end+1 to: end].
^(sortedSourceMap at: i) value "| method source scanner map | method := DebuggerMethodMap compiledMethodAt:
#rangeForPC:**contextIsActiveContext:. source := method getSourceFromFile asString. scanner := InstructionStream on: method. map := method debuggerMap. Array streamContents: [:ranges| [scanner atEnd] whileFalse: [| range | range := map rangeForPC: scanner pc contextIsActiveContext: true. ((map abstractSourceMap includesKey: scanner abstractPC) and: [range first ~= 0]) ifTrue: [ranges nextPut: (source copyFrom: range first to: range last)]. scanner interpretNextInstructionFor: InstructionClient new]]"!
-- best, Eliot
Eliot Miranda wrote:
On Thu, Sep 8, 2011 at 2:18 PM, Levente Uzonyi <leves@elte.hu mailto:leves@elte.hu> wrote:
On Thu, 8 Sep 2011, Eliot Miranda wrote: Levente, I object :) I find the findBinary:do:ifNone: extremely ugly and much prefer the findNearbyBinaryIndex: usage. Would you mind if we reverted to that? I pushed it to the inbox, so there's nothing to revert.
Ah, sorry. Saw the commit notice and assumed. Apologies.
I prefer to have as few implementations of binary search as possible.
Yes, but findNearbyBinaryIndex: pays its way. It saved 11 lines and is way more comprehensible.
Did you check the other changes? Are those acceptable?
I'm mulling them over :) I like a variable for the context pc (which Henrik didn't suggest) and I like using an at:ifAbsent: block. So I'll hopefully submit a new version sometime soon.
Levente P.S.: I just saw that Henrik wrote a mail about this method on the Pharo list with some other simplifications.
-- best, Eliot
You can also write something like
sortedSourceMap findBinaryIndex: [ :assoc | pc - assoc key ] do: [ :i | ^(sortedSourceMap at: i) value ] ifNone: [ :lower :upper | lower = 0 ifTrue: [ ^1 to: 0 ]. upper <= sortedSourceMap size ifTrue: [ ^(sortedSourceMap at: upper) value ]. "No match, but a nearby element." end := sortedSourceMap detectMax: [ :each | each value last ]. ^end + 1 to: end ]
that is only 3 lines longer, doesn't require a new binary search method, and looks clearer than the other 2 options to me.
Cheers, Juan Vuletich
On Thu, 8 Sep 2011, Eliot Miranda wrote:
On Thu, Sep 8, 2011 at 2:18 PM, Levente Uzonyi leves@elte.hu wrote:
On Thu, 8 Sep 2011, Eliot Miranda wrote:
Levente, I object :) I find the findBinary:do:ifNone: extremely ugly and
much prefer the findNearbyBinaryIndex: usage. Would you mind if we reverted to that?
I pushed it to the inbox, so there's nothing to revert.
Ah, sorry. Saw the commit notice and assumed. Apologies.
I prefer to have as few implementations of binary search as possible.
Yes, but findNearbyBinaryIndex: pays its way. It saved 11 lines and is way more comprehensible.
I didn't even try to save lines (which highly depend on indentation), but to use existing methods, simple blocks and the stack (no return from blocks).
To keep only one binary search implementation and also keep #findNearbyBinaryIndex:, I suggest a new implementation for it:
^self findBinaryIndex: aBlock ifNone: [ :lower :upper | lower = self size ifTrue: [ upper ] ifFalse: [ lower ] ]
This is only 6 lines long, but could be just one if you like it that way:
^self findBinaryIndex: aBlock ifNone: [ :l :u | l = self size ifTrue: [ u ] ifFalse: [ l ] ]
instead of 12. :)
Did you check the other changes? Are those acceptable?
I'm mulling them over :) I like a variable for the context pc (which Henrik didn't suggest) and I like using an at:ifAbsent: block. So I'll hopefully submit a new version sometime soon.
Thanks. I found that my code has a bug. #detectMax: returns the association instead of the maximum value according to the block, so some extra sends are necessary that way.
Levente
snip
squeak-dev@lists.squeakfoundation.org