Marcel Taeumel uploaded a new version of System to project The Inbox:
http://source.squeak.org/inbox/System-mt.697.mcz ==================== Summary ==================== Name: System-mt.697 Author: mt Time: 27 January 2015, 5:53:38.431 pm UUID: 9296f429-477d-0745-a878-6e11e7de5edd Ancestors: System-cmm.696 Let message tally ignore #home but just use #sender to handle block closures correctly. =============== Diff against System-cmm.696 =============== Item was changed: ----- Method: MessageTally>>tally:by: (in category 'tallying') ----- tally: context by: count "Explicitly tally the specified context and its stack." | sender | "Add to this node if appropriate" context method == method ifTrue: [^self bumpBy: count]. "No sender? Add new branch to the tree." + (sender := context sender) ifNil: [ - (sender := context home sender)ifNil: [ ^ (self bumpBy: count) tallyPath: context by: count]. "Find the node for the sending context (or add it if necessary)" ^ (self tally: sender by: count) tallyPath: context by: count! Item was changed: ----- Method: MessageTally>>tally:in:by: (in category 'tallying') ----- tally: context in: aProcess by: count "Explicitly tally the specified context and its stack." | sender | "Add to this node if appropriate" context method == method ifTrue: [^self bumpBy: count]. "No sender? Add new branch to the tree." + (sender := context sender) ifNil: [ - (sender := context home sender) ifNil: [ ^ (self bumpBy: count) tallyPath: context in: aProcess by: count]. "Find the node for the sending context (or add it if necessary)" ^ (self tally: sender in: aProcess by: count) tallyPath: context in: aProcess by: count! |
Is there any scenario, where #home is important for the tally?
Best, Marcel |
In reply to this post by commits-2
Wow, that's quite the bug :-) On Tue, Jan 27, 2015 at 8:53 AM, <[hidden email]> wrote: Marcel Taeumel uploaded a new version of System to project The Inbox: best,
Eliot |
In reply to this post by marcel.taeumel (old)
On Tue, Jan 27, 2015 at 8:51 AM, Marcel Taeumel <[hidden email]> wrote: Is there any scenario, where #home is important for the tally? Not as far as I can tell. I think you have it right. best,
Eliot |
In reply to this post by Eliot Miranda-2
On 27.01.2015, at 19:37, Eliot Miranda <[hidden email]> wrote:
Indeed. I wonder how this could have ever worked. Or maybe just nobody noticed that certain chains were missing? - Bert -
smime.p7s (5K) Download Attachment |
This is easy for an expert to glance at and understand the nature of
the bug but could someone explain it to help our Smalltalk-user community understand what has been wrong with MessageTally all these years? Thanks! On Tue, Jan 27, 2015 at 12:54 PM, Bert Freudenberg <[hidden email]> wrote: > On 27.01.2015, at 19:37, Eliot Miranda <[hidden email]> wrote: > > > Wow, that's quite the bug :-) > > > Indeed. I wonder how this could have ever worked. Or maybe just nobody > noticed that certain chains were missing? > > - Bert - > > > > On Tue, Jan 27, 2015 at 8:53 AM, <[hidden email]> wrote: >> >> Marcel Taeumel uploaded a new version of System to project The Inbox: >> http://source.squeak.org/inbox/System-mt.697.mcz >> >> ==================== Summary ==================== >> >> Name: System-mt.697 >> Author: mt >> Time: 27 January 2015, 5:53:38.431 pm >> UUID: 9296f429-477d-0745-a878-6e11e7de5edd >> Ancestors: System-cmm.696 >> >> Let message tally ignore #home but just use #sender to handle block >> closures correctly. >> >> =============== Diff against System-cmm.696 =============== >> >> Item was changed: >> ----- Method: MessageTally>>tally:by: (in category 'tallying') ----- >> tally: context by: count >> "Explicitly tally the specified context and its stack." >> | sender | >> >> "Add to this node if appropriate" >> context method == method ifTrue: [^self bumpBy: count]. >> >> "No sender? Add new branch to the tree." >> + (sender := context sender) ifNil: [ >> - (sender := context home sender)ifNil: [ >> ^ (self bumpBy: count) tallyPath: context by: count]. >> >> "Find the node for the sending context (or add it if necessary)" >> ^ (self tally: sender by: count) tallyPath: context by: count! >> >> Item was changed: >> ----- Method: MessageTally>>tally:in:by: (in category 'tallying') ----- >> tally: context in: aProcess by: count >> "Explicitly tally the specified context and its stack." >> | sender | >> >> "Add to this node if appropriate" >> context method == method ifTrue: [^self bumpBy: count]. >> >> "No sender? Add new branch to the tree." >> + (sender := context sender) ifNil: [ >> - (sender := context home sender) ifNil: [ >> ^ (self bumpBy: count) tallyPath: context in: aProcess by: >> count]. >> >> "Find the node for the sending context (or add it if necessary)" >> ^ (self tally: sender in: aProcess by: count) tallyPath: context >> in: aProcess by: count! >> >> > > > > -- > best, > Eliot > > > > > > |
AndreasSystemProfiler has the same bug, and it's annoyed me forever, so
thanks for the fix. I'm not an expert, but here's an example: AndreasSystemProfiler spyOn: [ [ #((1 2 3)) do: [ :each | each findLast: [ :ea | ea squared = ea ] ] ] bench ]. Before the patch you got: 99.98 (5,018) UndefinedObject DoIt 10.39 (521) Array [SequenceableCollection] findLast: 9.68 (486) Array [SequenceableCollection] do: 5.94 (298) Time class millisecondClockValue But the profiled block doesn't send #findLast, nor #do:, and neither #millisecondClockValue. It sends #bench to a block, and nothing else. After the fix it's: 99.98 (5,018) UndefinedObject DoIt 99.98 (5,018) BlockClosure bench 99.96 (5,017) UndefinedObject DoIt 20.9 (1,049) Array [SequenceableCollection] do: |16.01 (804) UndefinedObject DoIt | 16.01 (803) Array [SequenceableCollection] findLast: 5.7 (286) Time class millisecondClockValue Which is much better, and almost exact. #millisecondsClockValue is sent from #bench, not from a block. Levente On Tue, 27 Jan 2015, Chris Muller wrote: > This is easy for an expert to glance at and understand the nature of > the bug but could someone explain it to help our Smalltalk-user > community understand what has been wrong with MessageTally all these > years? > > Thanks! > > On Tue, Jan 27, 2015 at 12:54 PM, Bert Freudenberg <[hidden email]> wrote: >> On 27.01.2015, at 19:37, Eliot Miranda <[hidden email]> wrote: >> >> >> Wow, that's quite the bug :-) >> >> >> Indeed. I wonder how this could have ever worked. Or maybe just nobody >> noticed that certain chains were missing? >> >> - Bert - >> >> >> >> On Tue, Jan 27, 2015 at 8:53 AM, <[hidden email]> wrote: >>> >>> Marcel Taeumel uploaded a new version of System to project The Inbox: >>> http://source.squeak.org/inbox/System-mt.697.mcz >>> >>> ==================== Summary ==================== >>> >>> Name: System-mt.697 >>> Author: mt >>> Time: 27 January 2015, 5:53:38.431 pm >>> UUID: 9296f429-477d-0745-a878-6e11e7de5edd >>> Ancestors: System-cmm.696 >>> >>> Let message tally ignore #home but just use #sender to handle block >>> closures correctly. >>> >>> =============== Diff against System-cmm.696 =============== >>> >>> Item was changed: >>> ----- Method: MessageTally>>tally:by: (in category 'tallying') ----- >>> tally: context by: count >>> "Explicitly tally the specified context and its stack." >>> | sender | >>> >>> "Add to this node if appropriate" >>> context method == method ifTrue: [^self bumpBy: count]. >>> >>> "No sender? Add new branch to the tree." >>> + (sender := context sender) ifNil: [ >>> - (sender := context home sender)ifNil: [ >>> ^ (self bumpBy: count) tallyPath: context by: count]. >>> >>> "Find the node for the sending context (or add it if necessary)" >>> ^ (self tally: sender by: count) tallyPath: context by: count! >>> >>> Item was changed: >>> ----- Method: MessageTally>>tally:in:by: (in category 'tallying') ----- >>> tally: context in: aProcess by: count >>> "Explicitly tally the specified context and its stack." >>> | sender | >>> >>> "Add to this node if appropriate" >>> context method == method ifTrue: [^self bumpBy: count]. >>> >>> "No sender? Add new branch to the tree." >>> + (sender := context sender) ifNil: [ >>> - (sender := context home sender) ifNil: [ >>> ^ (self bumpBy: count) tallyPath: context in: aProcess by: >>> count]. >>> >>> "Find the node for the sending context (or add it if necessary)" >>> ^ (self tally: sender in: aProcess by: count) tallyPath: context >>> in: aProcess by: count! >>> >>> >> >> >> >> -- >> best, >> Eliot >> >> >> >> >> >> > > |
Hi,
On 27.01.2015, at 22:21, Levente Uzonyi <[hidden email]> wrote: > AndreasSystemProfiler This is not in trunk, right? If so, why? Best -Tobias |
Good question. It uses some primitives, which may not be supported by the
interpreter VM. Levente On Tue, 27 Jan 2015, Tobias Pape wrote: > Hi, > > On 27.01.2015, at 22:21, Levente Uzonyi <[hidden email]> wrote: > >> AndreasSystemProfiler > > This is not in trunk, right? > If so, why? > > Best > -Tobias > > > |
In reply to this post by Tobias Pape
On Tue, Jan 27, 2015 at 1:39 PM, Tobias Pape <[hidden email]> wrote: Hi, It came from Qwaq/Terf so it ended up in http://ss3.gemstone.com/ss/AndreasSystemProfiler. Further, as Levente says it depends on support that is in Cog VMs (JIT, Stack & Interpreter) but not (yet) in the trunk Interpreter. So I guess it doesn't belong in trunk until the Cog Interpreter has been merged with the trunk Interpreter. That's a fair amount of work, and neither David nor I have the time for it right now. best,
Eliot |
In reply to this post by Levente Uzonyi-2
On Tue, Jan 27, 2015 at 1:21 PM, Levente Uzonyi <[hidden email]> wrote: AndreasSystemProfiler has the same bug, and it's annoyed me forever, so thanks for the fix. There's a printing bug there. IMO it should look like 99.98 (5,018) UndefinedObject DoIt 99.98 (5,018) BlockClosure bench99.96 (5,017) [] in UndefinedObject DoIt 20.9 (1,049) Array [SequenceableCollection] do: |16.01 (804) [] in UndefinedObject DoIt | 16.01 (803) Array [SequenceableCollection] findLast: 5.7 (286) Time class millisecondClockValue I'll take a look.
best,
Eliot |
In reply to this post by Levente Uzonyi-2
> On 27.01.2015, at 22:21, Levente Uzonyi <[hidden email]> wrote:
> > AndreasSystemProfiler has the same bug, and it's annoyed me forever, so thanks for the fix. Yep, Marcel is on a roll. Keep it coming! :) - Bert - smime.p7s (8K) Download Attachment |
In reply to this post by Eliot Miranda-2
On Tue, Jan 27, 2015 at 01:51:52PM -0800, Eliot Miranda wrote:
> > On Tue, Jan 27, 2015 at 1:39 PM, Tobias Pape <[hidden email]> wrote: > > > Hi, > > > > On 27.01.2015, at 22:21, Levente Uzonyi <[hidden email]> wrote: > > > > > AndreasSystemProfiler > > > > This is not in trunk, right? > > If so, why? > > > > It came from Qwaq/Terf so it ended up in > http://ss3.gemstone.com/ss/AndreasSystemProfiler. Further, as Levente says > it depends on support that is in Cog VMs (JIT, Stack & Interpreter) but not > (yet) in the trunk Interpreter. So I guess it doesn't belong in trunk > until the Cog Interpreter has been merged with the trunk Interpreter. > That's a fair amount of work, and neither David nor I have the time for it > right now. I did spend some time last year trying to get the primitives working in trunk Interpreter, but I was a bit out of my depth and did not get it right. Reminder and background info is on Mantis at http://bugs.squeak.org/view.php?id=7746 As far as putting the profiler into trunk, I should think that there would be some way to have more than one profiler, and fall back to the old one if the VM does not provide the needed support for AndreasSystemProfiler. This seems like a worthwhile thing to do. Anyone care to take it on? Dave |
On Jan 27, 2015, at 4:00 PM, "David T. Lewis" <[hidden email]> wrote: > > On Tue, Jan 27, 2015 at 01:51:52PM -0800, Eliot Miranda wrote: >> >> On Tue, Jan 27, 2015 at 1:39 PM, Tobias Pape <[hidden email]> wrote: >> >>> Hi, >>> >>> On 27.01.2015, at 22:21, Levente Uzonyi <[hidden email]> wrote: >>> >>>> AndreasSystemProfiler >>> >>> This is not in trunk, right? >>> If so, why? >> >> It came from Qwaq/Terf so it ended up in >> http://ss3.gemstone.com/ss/AndreasSystemProfiler. Further, as Levente says >> it depends on support that is in Cog VMs (JIT, Stack & Interpreter) but not >> (yet) in the trunk Interpreter. So I guess it doesn't belong in trunk >> until the Cog Interpreter has been merged with the trunk Interpreter. >> That's a fair amount of work, and neither David nor I have the time for it >> right now. > > I did spend some time last year trying to get the primitives working in > trunk Interpreter, but I was a bit out of my depth and did not get it right. Since the code is in the Cog Interpreter the right way to do this IMO us to port bug fixes to primitives etc into the Cog Interpreter and call the Cog VMMaker trunk. Going the other way takes longer to get to the same place. > > Reminder and background info is on Mantis at http://bugs.squeak.org/view.php?id=7746 > > As far as putting the profiler into trunk, I should think that there would > be some way to have more than one profiler, and fall back to the old one > if the VM does not provide the needed support for AndreasSystemProfiler. > > This seems like a worthwhile thing to do. Anyone care to take it on? > > Dave > |
On Tue, Jan 27, 2015 at 04:14:24PM -0800, Eliot Miranda wrote:
> On Jan 27, 2015, at 4:00 PM, "David T. Lewis" <[hidden email]> wrote: > > On Tue, Jan 27, 2015 at 01:51:52PM -0800, Eliot Miranda wrote: > >> On Tue, Jan 27, 2015 at 1:39 PM, Tobias Pape <[hidden email]> wrote: > >>> On 27.01.2015, at 22:21, Levente Uzonyi <[hidden email]> wrote: > >>> > >>>> AndreasSystemProfiler > >>> > >>> This is not in trunk, right? > >>> If so, why? > >> > >> It came from Qwaq/Terf so it ended up in > >> http://ss3.gemstone.com/ss/AndreasSystemProfiler. Further, as Levente says > >> it depends on support that is in Cog VMs (JIT, Stack & Interpreter) but not > >> (yet) in the trunk Interpreter. So I guess it doesn't belong in trunk > >> until the Cog Interpreter has been merged with the trunk Interpreter. > >> That's a fair amount of work, and neither David nor I have the time for it > >> right now. > > > > I did spend some time last year trying to get the primitives working in > > trunk Interpreter, but I was a bit out of my depth and did not get it right. > > Since the code is in the Cog Interpreter the right way to do this IMO us > to port bug fixes to primitives etc into the Cog Interpreter and call the > Cog VMMaker trunk. Going the other way takes longer to get to the same place. > Hi Eliot, Was Andreas' profiler fully functional on an Interpreter generated from oscog at one point? I know that Andreas had done at least an initial implementation in that part of the code. I had assumed that his main focus in that time frame would have been on Cog, so I was not entirely sure if the Interpreter implementation was complete. Of course I was trying to adopt the implementation from the oscog branch when I looked at this last year. But given that I was not successful, I won't attempt to argue that this was the best possible approach. Thanks, Dave |
On Tue, Jan 27, 2015 at 7:15 PM, David T. Lewis <[hidden email]> wrote:
When I joined Qwaq, Andreas' fork of VMMaker and svn trunk was the VM in use. The Interpreter contained all of the support for Andreas' profiler. That VMMaker was my starting point for Cog and I simply supported in the Stack and Cog VMs what was in the Interpreter. There was much else in there too; support for a high-priority media=-processing thread which was used for Qwaq's voice channel; extensions to the host window plugin etc, etc. This had to continue working as the VM was sped-up using first the Stack VM and then Cog. I guess the Qwaq VM forked from trunk in about 2005.
best,
Eliot |
On Tue, Jan 27, 2015 at 07:43:27PM -0800, Eliot Miranda wrote:
> On Tue, Jan 27, 2015 at 7:15 PM, David T. Lewis <[hidden email]> wrote: > > > > On Tue, Jan 27, 2015 at 04:14:24PM -0800, Eliot Miranda wrote: > > > > Was Andreas' profiler fully functional on an Interpreter generated from oscog > > at one point? I know that Andreas had done at least an initial implementation > > in that part of the code. I had assumed that his main focus in that time frame > > would have been on Cog, so I was not entirely sure if the Interpreter > > implementation was complete. > > > > When I joined Qwaq, Andreas' fork of VMMaker and svn trunk was the VM in > use. The Interpreter contained all of the support for Andreas' profiler. > That VMMaker was my starting point for Cog and I simply supported in the > Stack and Cog VMs what was in the Interpreter. > > There was much else in there too; support for a high-priority > media=-processing thread which was used for Qwaq's voice channel; > extensions to the host window plugin etc, etc. This had to continue > working as the VM was sped-up using first the Stack VM and then Cog. I > guess the Qwaq VM forked from trunk in about 2005. > Thanks Eliot, That probably sounded like a dumb question, but I actually was not sure if I was looking at a complete and functional implemention, or just a rough draft. Obviously it would have made more sense for me to have asked the question a year or so ago. I can't look into it further now, but the issue remains documented on Mantis at http://bugs.squeak.org/view.php?id=7746 so hopefully either I or some other motivated person will follow through on it in the not too distant future. Dave > > > Of course I was trying to adopt the implementation from the oscog branch > > when I looked at this last year. But given that I was not successful, I > > won't attempt to argue that this was the best possible approach. > > > > Thanks, > > Dave > > > > -- > best, > Eliot > |
In reply to this post by Eliot Miranda-2
On 27.01.2015, at 22:55, Eliot Miranda <[hidden email]> wrote:
Tallies currently only count per-method, not per-block. Maybe that is the reason why previously it tried to go via a block’s home? - Bert - smime.p7s (5K) Download Attachment |
If we keep record of MethodContext >> #isExecutingBlock in MessageTally >> #tallyPath:by: (resp. #tallyPath:in:by:) we could change the printing to prepend "[] in" again. :)
Best, Marcel |
In reply to this post by Bert Freudenberg
Hi Bert,
|
Free forum by Nabble | Edit this page |