The Trunk: Tools-eem.994.mcz

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

The Trunk: Tools-eem.994.mcz

commits-2
Eliot Miranda uploaded a new version of Tools to project The Trunk:
http://source.squeak.org/trunk/Tools-eem.994.mcz

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

Name: Tools-eem.994
Author: eem
Time: 2 October 2020, 1:44:29.015648 pm
UUID: 23145257-280d-4a4a-a4ba-3b5cc367f9a9
Ancestors: Tools-eem.993

Oops! Compiledmethod>>blockExtentsToTempsMap needs to observe that startKeysToBlockExtents has moved to DebuggerMethodMap.

=============== Diff against Tools-eem.993 ===============

Item was changed:
  ----- Method: CompiledMethod>>blockExtentsToTempsMap (in category '*Tools-Debugger-support') -----
  blockExtentsToTempsMap
  "If the receiver has been copied with temp names answer a
  map from blockExtent to temps map in the same format as
  BytecodeEncoder>>blockExtentsToTempNamesMap.  if the
  receiver has not been copied with temps answer nil."
  ^self holdsTempNames ifTrue:
+ [self mapFromBlockKeys: (self debuggerMap startKeysToBlockExtents values sort: [:assocA :assocB| assocA first <= assocB first])
- [self mapFromBlockKeys: (self startKeysToBlockExtents values sort: [:assocA :assocB| assocA first <= assocB first])
  toSchematicTemps: self tempNamesString]!


Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Tools-eem.994.mcz

Eliot Miranda-2
Hi Marcel, Hi Christoph,

    I emailed the list and cc'ed you to get your attention.  Forgive my rude interruption.  I finally found out where the slow down really is.  It is in 

Inspector>>fieldList
"Return a list of texts that identify the fields for the object under inspection so that the user can make an informed decision on what to inspect."
^ self fieldListStyler
ifNil: [self fields collect: [:field | field name]]
ifNotNil: [:styler |
self updateStyler: styler.
self fields collect: [:field |
field shouldStyleName
ifTrue: [styler styledTextFor: field name asText]
ifFalse: [field name]]]

So this runs a styler over the entire method every time one steps.  And if one is stepping through a doit it will call the decompiler to generate the source to style, every time you step.  We have to do better :-)

Here's how to profile it.  Debug a doit.  I wrote this one:

| t |
t := 0.
[| a b c |
a := 1. b := 2. c := 100.
(a = 1 and: [b = 2 and: [c = 100]])
ifTrue:
[1 to: 100 by: 2 do:
[:i| t := t + 1]]
ifFalse:
[a to: c by: b do:
[:i| t := t + 1]]] repeat.
t

Once in the debugger inspect the "Over" button.  Then in that inspector evaluate 

      AndreasSystemProfiler spyOn: [1000 timesRepeat: [self performAction]]

and you'll see that essentially all the time is going into SHTextStylerST80(SHTextStyler) styledTextFor:

On Fri, Oct 2, 2020 at 1:44 PM <[hidden email]> wrote:
Eliot Miranda uploaded a new version of Tools to project The Trunk:
http://source.squeak.org/trunk/Tools-eem.994.mcz

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

Name: Tools-eem.994
Author: eem
Time: 2 October 2020, 1:44:29.015648 pm
UUID: 23145257-280d-4a4a-a4ba-3b5cc367f9a9
Ancestors: Tools-eem.993

Oops! Compiledmethod>>blockExtentsToTempsMap needs to observe that startKeysToBlockExtents has moved to DebuggerMethodMap.

=============== Diff against Tools-eem.993 ===============

Item was changed:
  ----- Method: CompiledMethod>>blockExtentsToTempsMap (in category '*Tools-Debugger-support') -----
  blockExtentsToTempsMap
        "If the receiver has been copied with temp names answer a
         map from blockExtent to temps map in the same format as
         BytecodeEncoder>>blockExtentsToTempNamesMap.  if the
         receiver has not been copied with temps answer nil."
        ^self holdsTempNames ifTrue:
+               [self mapFromBlockKeys: (self debuggerMap startKeysToBlockExtents values sort: [:assocA :assocB| assocA first <= assocB first])
-               [self mapFromBlockKeys: (self startKeysToBlockExtents values sort: [:assocA :assocB| assocA first <= assocB first])
                        toSchematicTemps: self tempNamesString]!




--
_,,,^..^,,,_
best, Eliot


Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Tools-eem.994.mcz

Christoph Thiede

Hi Eliot,


thanks for the exact steps to reproduce. I will use [1000 timesRepeat: [self performAction]] timeProfile because I don't have the AndreasSystemProfiler.


Does ShoutCore-ct.78 from the inbox help you to speed things up? For me, it speeds up the call from 3.84 sec down to 1.07 sec - but still, there happens a lot of redundant shout styling.

Hm, why can't we recreate the fields lazily in #fields instead of doing this eagerly in #resetFields? This is how I originally implemented it but then Marcel changed it - I don't know the reason. :-)


Best,

Christoph


Von: Eliot Miranda <[hidden email]>
Gesendet: Freitag, 2. Oktober 2020 22:59:04
An: The general-purpose Squeak developers list; Taeumel, Marcel; Thiede, Christoph
Cc: [hidden email]
Betreff: Re: [squeak-dev] The Trunk: Tools-eem.994.mcz
 
Hi Marcel, Hi Christoph,

    I emailed the list and cc'ed you to get your attention.  Forgive my rude interruption.  I finally found out where the slow down really is.  It is in 

Inspector>>fieldList
"Return a list of texts that identify the fields for the object under inspection so that the user can make an informed decision on what to inspect."
^ self fieldListStyler
ifNil: [self fields collect: [:field | field name]]
ifNotNil: [:styler |
self updateStyler: styler.
self fields collect: [:field |
field shouldStyleName
ifTrue: [styler styledTextFor: field name asText]
ifFalse: [field name]]]

So this runs a styler over the entire method every time one steps.  And if one is stepping through a doit it will call the decompiler to generate the source to style, every time you step.  We have to do better :-)

Here's how to profile it.  Debug a doit.  I wrote this one:

| t |
t := 0.
[| a b c |
a := 1. b := 2. c := 100.
(a = 1 and: [b = 2 and: [c = 100]])
ifTrue:
[1 to: 100 by: 2 do:
[:i| t := t + 1]]
ifFalse:
[a to: c by: b do:
[:i| t := t + 1]]] repeat.
t

Once in the debugger inspect the "Over" button.  Then in that inspector evaluate 

      AndreasSystemProfiler spyOn: [1000 timesRepeat: [self performAction]]

and you'll see that essentially all the time is going into SHTextStylerST80(SHTextStyler) styledTextFor:

On Fri, Oct 2, 2020 at 1:44 PM <[hidden email]> wrote:
Eliot Miranda uploaded a new version of Tools to project The Trunk:
http://source.squeak.org/trunk/Tools-eem.994.mcz

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

Name: Tools-eem.994
Author: eem
Time: 2 October 2020, 1:44:29.015648 pm
UUID: 23145257-280d-4a4a-a4ba-3b5cc367f9a9
Ancestors: Tools-eem.993

Oops! Compiledmethod>>blockExtentsToTempsMap needs to observe that startKeysToBlockExtents has moved to DebuggerMethodMap.

=============== Diff against Tools-eem.993 ===============

Item was changed:
  ----- Method: CompiledMethod>>blockExtentsToTempsMap (in category '*Tools-Debugger-support') -----
  blockExtentsToTempsMap
        "If the receiver has been copied with temp names answer a
         map from blockExtent to temps map in the same format as
         BytecodeEncoder>>blockExtentsToTempNamesMap.  if the
         receiver has not been copied with temps answer nil."
        ^self holdsTempNames ifTrue:
+               [self mapFromBlockKeys: (self debuggerMap startKeysToBlockExtents values sort: [:assocA :assocB| assocA first <= assocB first])
-               [self mapFromBlockKeys: (self startKeysToBlockExtents values sort: [:assocA :assocB| assocA first <= assocB first])
                        toSchematicTemps: self tempNamesString]!




--
_,,,^..^,,,_
best, Eliot


Carpe Squeak!
Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Tools-eem.994.mcz

timrowledge
Forgive the potentially dumb question, but why exactly does one want to use any sort of styling stuff on fields in an inspector?

And if one *does* feel a need for that, since we know quite a lot about what the field labels are, why can one not do some more focussed action than a general 'err, here, style this thing for me'?

> On 2020-10-02, at 2:18 PM, Thiede, Christoph <[hidden email]> wrote:
>
> Hi Eliot,
>
> thanks for the exact steps to reproduce. I will use [1000 timesRepeat: [self performAction]] timeProfile because I don't have the AndreasSystemProfiler.
>
> Does ShoutCore-ct.78 from the inbox help you to speed things up? For me, it speeds up the call from 3.84 sec down to 1.07 sec - but still, there happens a lot of redundant shout styling.
> Hm, why can't we recreate the fields lazily in #fields instead of doing this eagerly in #resetFields? This is how I originally implemented it but then Marcel changed it - I don't know the reason. :-)
>
> Best,
> Christoph
> Von: Eliot Miranda <[hidden email]>
> Gesendet: Freitag, 2. Oktober 2020 22:59:04
> An: The general-purpose Squeak developers list; Taeumel, Marcel; Thiede, Christoph
> Cc: [hidden email]
> Betreff: Re: [squeak-dev] The Trunk: Tools-eem.994.mcz
>  
> Hi Marcel, Hi Christoph,
>
>     I emailed the list and cc'ed you to get your attention.  Forgive my rude interruption.  I finally found out where the slow down really is.  It is in
>
> Inspector>>fieldList
> "Return a list of texts that identify the fields for the object under inspection so that the user can make an informed decision on what to inspect."
> ^ self fieldListStyler
> ifNil: [self fields collect: [:field | field name]]
> ifNotNil: [:styler |
> self updateStyler: styler.
> self fields collect: [:field |
> field shouldStyleName
> ifTrue: [styler styledTextFor: field name asText]
> ifFalse: [field name]]]
>
> So this runs a styler over the entire method every time one steps.  And if one is stepping through a doit it will call the decompiler to generate the source to style, every time you step.  We have to do better :-)
>
> Here's how to profile it.  Debug a doit.  I wrote this one:
>
> | t |
> t := 0.
> [| a b c |
> a := 1. b := 2. c := 100.
> (a = 1 and: [b = 2 and: [c = 100]])
> ifTrue:
> [1 to: 100 by: 2 do:
> [:i| t := t + 1]]
> ifFalse:
> [a to: c by: b do:
> [:i| t := t + 1]]] repeat.
> t
>
> Once in the debugger inspect the "Over" button.  Then in that inspector evaluate
>
>       AndreasSystemProfiler spyOn: [1000 timesRepeat: [self performAction]]
>
> and you'll see that essentially all the time is going into SHTextStylerST80(SHTextStyler) styledTextFor:
>
> On Fri, Oct 2, 2020 at 1:44 PM <[hidden email]> wrote:
> Eliot Miranda uploaded a new version of Tools to project The Trunk:
> http://source.squeak.org/trunk/Tools-eem.994.mcz
>
> ==================== Summary ====================
>
> Name: Tools-eem.994
> Author: eem
> Time: 2 October 2020, 1:44:29.015648 pm
> UUID: 23145257-280d-4a4a-a4ba-3b5cc367f9a9
> Ancestors: Tools-eem.993
>
> Oops! Compiledmethod>>blockExtentsToTempsMap needs to observe that startKeysToBlockExtents has moved to DebuggerMethodMap.
>
> =============== Diff against Tools-eem.993 ===============
>
> Item was changed:
>   ----- Method: CompiledMethod>>blockExtentsToTempsMap (in category '*Tools-Debugger-support') -----
>   blockExtentsToTempsMap
>         "If the receiver has been copied with temp names answer a
>          map from blockExtent to temps map in the same format as
>          BytecodeEncoder>>blockExtentsToTempNamesMap.  if the
>          receiver has not been copied with temps answer nil."
>         ^self holdsTempNames ifTrue:
> +               [self mapFromBlockKeys: (self debuggerMap startKeysToBlockExtents values sort: [:assocA :assocB| assocA first <= assocB first])
> -               [self mapFromBlockKeys: (self startKeysToBlockExtents values sort: [:assocA :assocB| assocA first <= assocB first])
>                         toSchematicTemps: self tempNamesString]!
>
>
>
>
> --
> _,,,^..^,,,_
> best, Eliot


tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Do files get embarrassed when they get unzipped?



Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Tools-eem.994.mcz

Christoph Thiede

Hi Tim,


I added the styling to the inspector fields because, in my opinion, this helps you to identify different kinds of fields faster. But we should start honoring the preference SHTextStylerST80 syntaxHighlightingAsYouType again, this would allow you to turn off styling in your individual image.


And if one *does* feel a need for that, since we know quite a lot about what the field labels are, why can one not do some more focussed action than a general 'err, here, style this thing for me'?


This was simply a question of code quality vs. optimization. One main objective of the recent inspector refactoring was to make it easier to extend/subclass it, and reproducing the styler logic would make this unnecessarily complicated. Also, the styling must honor be updated when the user interface theme is changed, so using Shout out of the box simply looked like the most straightforward and elegant solution to me ...

We could also introduce a cache of the styled field titles in Inspector >> #resetFields by computing a hash value from the list of unstyled field titles.

Best,
Christoph

Von: Squeak-dev <[hidden email]> im Auftrag von tim Rowledge <[hidden email]>
Gesendet: Freitag, 2. Oktober 2020 23:37:02
An: The general-purpose Squeak developers list
Cc: [hidden email]
Betreff: Re: [squeak-dev] The Trunk: Tools-eem.994.mcz
 
Forgive the potentially dumb question, but why exactly does one want to use any sort of styling stuff on fields in an inspector?

And if one *does* feel a need for that, since we know quite a lot about what the field labels are, why can one not do some more focussed action than a general 'err, here, style this thing for me'?

> On 2020-10-02, at 2:18 PM, Thiede, Christoph <[hidden email]> wrote:
>
> Hi Eliot,
>
> thanks for the exact steps to reproduce. I will use [1000 timesRepeat: [self performAction]] timeProfile because I don't have the AndreasSystemProfiler.
>
> Does ShoutCore-ct.78 from the inbox help you to speed things up? For me, it speeds up the call from 3.84 sec down to 1.07 sec - but still, there happens a lot of redundant shout styling.
> Hm, why can't we recreate the fields lazily in #fields instead of doing this eagerly in #resetFields? This is how I originally implemented it but then Marcel changed it - I don't know the reason. :-)
>
> Best,
> Christoph
> Von: Eliot Miranda <[hidden email]>
> Gesendet: Freitag, 2. Oktober 2020 22:59:04
> An: The general-purpose Squeak developers list; Taeumel, Marcel; Thiede, Christoph
> Cc: [hidden email]
> Betreff: Re: [squeak-dev] The Trunk: Tools-eem.994.mcz

> Hi Marcel, Hi Christoph,
>
>     I emailed the list and cc'ed you to get your attention.  Forgive my rude interruption.  I finally found out where the slow down really is.  It is in
>
> Inspector>>fieldList
> "Return a list of texts that identify the fields for the object under inspection so that the user can make an informed decision on what to inspect."
> ^ self fieldListStyler
> ifNil: [self fields collect: [:field | field name]]
> ifNotNil: [:styler |
> self updateStyler: styler.
> self fields collect: [:field |
> field shouldStyleName
> ifTrue: [styler styledTextFor: field name asText]
> ifFalse: [field name]]]
>
> So this runs a styler over the entire method every time one steps.  And if one is stepping through a doit it will call the decompiler to generate the source to style, every time you step.  We have to do better :-)
>
> Here's how to profile it.  Debug a doit.  I wrote this one:
>
> | t |
> t := 0.
> [| a b c |
> a := 1. b := 2. c := 100.
> (a = 1 and: [b = 2 and: [c = 100]])
> ifTrue:
> [1 to: 100 by: 2 do:
> [:i| t := t + 1]]
> ifFalse:
> [a to: c by: b do:
> [:i| t := t + 1]]] repeat.
> t
>
> Once in the debugger inspect the "Over" button.  Then in that inspector evaluate
>
>       AndreasSystemProfiler spyOn: [1000 timesRepeat: [self performAction]]
>
> and you'll see that essentially all the time is going into SHTextStylerST80(SHTextStyler) styledTextFor:
>
> On Fri, Oct 2, 2020 at 1:44 PM <[hidden email]> wrote:
> Eliot Miranda uploaded a new version of Tools to project The Trunk:
> http://source.squeak.org/trunk/Tools-eem.994.mcz
>
> ==================== Summary ====================
>
> Name: Tools-eem.994
> Author: eem
> Time: 2 October 2020, 1:44:29.015648 pm
> UUID: 23145257-280d-4a4a-a4ba-3b5cc367f9a9
> Ancestors: Tools-eem.993
>
> Oops! Compiledmethod>>blockExtentsToTempsMap needs to observe that startKeysToBlockExtents has moved to DebuggerMethodMap.
>
> =============== Diff against Tools-eem.993 ===============
>
> Item was changed:
>   ----- Method: CompiledMethod>>blockExtentsToTempsMap (in category '*Tools-Debugger-support') -----
>   blockExtentsToTempsMap
>         "If the receiver has been copied with temp names answer a
>          map from blockExtent to temps map in the same format as
>          BytecodeEncoder>>blockExtentsToTempNamesMap.  if the
>          receiver has not been copied with temps answer nil."
>         ^self holdsTempNames ifTrue:
> +               [self mapFromBlockKeys: (self debuggerMap startKeysToBlockExtents values sort: [:assocA :assocB| assocA first <= assocB first])
> -               [self mapFromBlockKeys: (self startKeysToBlockExtents values sort: [:assocA :assocB| assocA first <= assocB first])
>                         toSchematicTemps: self tempNamesString]!
>
>
>
>
> --
> _,,,^..^,,,_
> best, Eliot


tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Do files get embarrassed when they get unzipped?





Carpe Squeak!
Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Tools-eem.994.mcz

marcel.taeumel
Interesting! Yes, there is redundant work happening when styling field-by-field. Hmmm... one could add some kind cache shared between all "inspector fields" in an inspector. That stepping is not the issue here because debugger inspectors do not step.

Best,
Marcel

Am 03.10.2020 00:07:35 schrieb Thiede, Christoph <[hidden email]>:

Hi Tim,


I added the styling to the inspector fields because, in my opinion, this helps you to identify different kinds of fields faster. But we should start honoring the preference SHTextStylerST80 syntaxHighlightingAsYouType again, this would allow you to turn off styling in your individual image.


And if one *does* feel a need for that, since we know quite a lot about what the field labels are, why can one not do some more focussed action than a general 'err, here, style this thing for me'?


This was simply a question of code quality vs. optimization. One main objective of the recent inspector refactoring was to make it easier to extend/subclass it, and reproducing the styler logic would make this unnecessarily complicated. Also, the styling must honor be updated when the user interface theme is changed, so using Shout out of the box simply looked like the most straightforward and elegant solution to me ...

We could also introduce a cache of the styled field titles in Inspector >> #resetFields by computing a hash value from the list of unstyled field titles.

Best,
Christoph

Von: Squeak-dev <[hidden email]> im Auftrag von tim Rowledge <[hidden email]>
Gesendet: Freitag, 2. Oktober 2020 23:37:02
An: The general-purpose Squeak developers list
Cc: [hidden email]
Betreff: Re: [squeak-dev] The Trunk: Tools-eem.994.mcz
 
Forgive the potentially dumb question, but why exactly does one want to use any sort of styling stuff on fields in an inspector?

And if one *does* feel a need for that, since we know quite a lot about what the field labels are, why can one not do some more focussed action than a general 'err, here, style this thing for me'?

> On 2020-10-02, at 2:18 PM, Thiede, Christoph <[hidden email]> wrote:
>
> Hi Eliot,
>
> thanks for the exact steps to reproduce. I will use [1000 timesRepeat: [self performAction]] timeProfile because I don't have the AndreasSystemProfiler.
>
> Does ShoutCore-ct.78 from the inbox help you to speed things up? For me, it speeds up the call from 3.84 sec down to 1.07 sec - but still, there happens a lot of redundant shout styling.
> Hm, why can't we recreate the fields lazily in #fields instead of doing this eagerly in #resetFields? This is how I originally implemented it but then Marcel changed it - I don't know the reason. :-)
>
> Best,
> Christoph
> Von: Eliot Miranda <[hidden email]>
> Gesendet: Freitag, 2. Oktober 2020 22:59:04
> An: The general-purpose Squeak developers list; Taeumel, Marcel; Thiede, Christoph
> Cc: [hidden email]
> Betreff: Re: [squeak-dev] The Trunk: Tools-eem.994.mcz

> Hi Marcel, Hi Christoph,
>
>     I emailed the list and cc'ed you to get your attention.  Forgive my rude interruption.  I finally found out where the slow down really is.  It is in
>
> Inspector>>fieldList
> "Return a list of texts that identify the fields for the object under inspection so that the user can make an informed decision on what to inspect."
> ^ self fieldListStyler
> ifNil: [self fields collect: [:field | field name]]
> ifNotNil: [:styler |
> self updateStyler: styler.
> self fields collect: [:field |
> field shouldStyleName
> ifTrue: [styler styledTextFor: field name asText]
> ifFalse: [field name]]]
>
> So this runs a styler over the entire method every time one steps.  And if one is stepping through a doit it will call the decompiler to generate the source to style, every time you step.  We have to do better :-)
>
> Here's how to profile it.  Debug a doit.  I wrote this one:
>
> | t |
> t := 0.
> [| a b c |
> a := 1. b := 2. c := 100.
> (a = 1 and: [b = 2 and: [c = 100]])
> ifTrue:
> [1 to: 100 by: 2 do:
> [:i| t := t + 1]]
> ifFalse:
> [a to: c by: b do:
> [:i| t := t + 1]]] repeat.
> t
>
> Once in the debugger inspect the "Over" button.  Then in that inspector evaluate
>
>       AndreasSystemProfiler spyOn: [1000 timesRepeat: [self performAction]]
>
> and you'll see that essentially all the time is going into SHTextStylerST80(SHTextStyler) styledTextFor:
>
> On Fri, Oct 2, 2020 at 1:44 PM <[hidden email]> wrote:
> Eliot Miranda uploaded a new version of Tools to project The Trunk:
> http://source.squeak.org/trunk/Tools-eem.994.mcz
>
> ==================== Summary ====================
>
> Name: Tools-eem.994
> Author: eem
> Time: 2 October 2020, 1:44:29.015648 pm
> UUID: 23145257-280d-4a4a-a4ba-3b5cc367f9a9
> Ancestors: Tools-eem.993
>
> Oops! Compiledmethod>>blockExtentsToTempsMap needs to observe that startKeysToBlockExtents has moved to DebuggerMethodMap.
>
> =============== Diff against Tools-eem.993 ===============
>
> Item was changed:
>   ----- Method: CompiledMethod>>blockExtentsToTempsMap (in category '*Tools-Debugger-support') -----
>   blockExtentsToTempsMap
>         "If the receiver has been copied with temp names answer a
>          map from blockExtent to temps map in the same format as
>          BytecodeEncoder>>blockExtentsToTempNamesMap.  if the
>          receiver has not been copied with temps answer nil."
>         ^self holdsTempNames ifTrue:
> +               [self mapFromBlockKeys: (self debuggerMap startKeysToBlockExtents values sort: [:assocA :assocB| assocA first <= assocB first])
> -               [self mapFromBlockKeys: (self startKeysToBlockExtents values sort: [:assocA :assocB| assocA first <= assocB first])
>                         toSchematicTemps: self tempNamesString]!
>
>
>
>
> --
> _,,,^..^,,,_
> best, Eliot


tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Do files get embarrassed when they get unzipped?





Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Tools-eem.994.mcz

Christoph Thiede
In reply to this post by commits-2

Hi Eliot,


sorry for the late reply, and sorry for the private message, I just hit "Reply all" ... We're back on list now. :-)


Your answer contains many important insights!


Does my perspective make sense or does it feel wrong to you?


I see you want to make the debugger robust and minimal. Still, I have the feeling that we would need to reinvent a piece of the whole if styled the fields manually. Wouldn't this raise the complexity again and open the doors way to new bugs?
And yes, I admit that I ran a number of times into undebuggable situations because the Decompiler was broken for the method I tried to debug, which is an undesired behavior, of course. But a) in this option I'd be fine to turn off a preference and b) we can just wrap the calls to the styler from the Inspector into an #ifError: block if we want to.
Would such an approach satisfy your criteria of robustness? :-)

> > If you debug code in a collection, you will see a collection inspector ... if you debug code in a Morph, you will see a MorphInspector etc. ... See the whole inspector hierarchy and implementors of #inspectorClass. I have the impression that optimizing this on the level you are proposing would be a quite excessive optimization. :-)
> Hmm, I see.  This is a major change in semantics, and IMO in general not a good one.  Inside a method one is seeing the raw representation of an object.  The receiver inspector used to present the view that accorded with the method, showing inside the object.  I can u set stand why one might want the external view but at the very least this should be a preference.  Ideally one would be able to toggle the preference on an individual debugger.  But, for example, if I’m debugging some internals in Dictionary et al I *need* to see that internal state in the debugger.
> Note that if I want to see the external state I can always spawn a normal inspector on self.  I can’t do the opposite easily; I have to send basicInspect.

Well, I never considered that argument, but it's a justified one, of course. I took advantage of the "arbitrary" receiver inspectors a number of times, e. g. when debugging through a morph tree and identifying the morphs quickly by selecting the screenshot field. For some cases, I'd even wish to nest inspectors or embed explorers into the debugger; but yes, all of this can slow down or destabilize the debugging experience ...
So maybe we are searching something like a "BasicDebugger"? Should this become an extra concept or would this be misleading?
The simplest solution I could imagine would be a new toggle switch in the debugger's window menu (at the right top corner), something like "<off> basic mode". If enabled, we could use a regular inspector instead of an arbitrary (that's literally one selector we have to exchange), and if you think it would be helpful, we could also turn off styling in this case. However, I believe the latter should rather be an automatic fallback via #ifError:. Alternatively, this switch could also go into the inspector's field list menu as well, maybe this would be even more straightforward?

Everyone might feel free to send proposals of any degree; if nothing happens, I will try to plan this for somewhen this month.

@Marcel:

Hmmm... one could add some kind cache shared between all "inspector fields" in an inspector.

Why not cache the styling results directly in the styler?

Best,
Christoph


Von: Eliot Miranda <[hidden email]>
Gesendet: Samstag, 3. Oktober 2020 01:46:01
An: Thiede, Christoph
Betreff: Re: [squeak-dev] The Trunk: Tools-eem.994.mcz
 
Hi Christoph,

On Oct 2, 2020, at 3:32 PM, Thiede, Christoph <[hidden email]> wrote:



Hi Eliot,


as proposed on the list, wouldn't it be enough to cache the styled field titles if the unstyled field titles didn't change? Or would it be helpful to implement a general cache for every text passed to shout directly in the styler class?


The thing about caching is that it’s slow for the first click.  And in general this highlighting could be so much simpler if this unnecessary parsing is avoided.  But it’s your code.

Why do you not want to discuss on list?

Here’s a thing to think about with debuggers, and if you look at Pharo you’ll find they’ve strayed too far the wrong way.  In a reflective system such as Smalltalk there is a tension between having a rich debugger experience and having a reliable debugger.  The richer the debugger’s functionality the greater the surface over which bugs can affect and break the debugger.  The same goes for the compiler.  It used to be that the compiler used only whileTrue: rather than both whileTrue: & whileFalse: (IIRC) to avoid the compiler depending on a particular kind of branch (long branch on false perhaps? There was an asymmetry in the blue book bytecode set’s long jump bytecodes, there was only a long branch false, and so by using one kind of loop this byte code wasn’t needed by the compiler.  You get the idea.  

And of course that’s why there is an emergency evaluator, because historically people broke debugging or opening a debugger (halts in Croquet’s teatime protocol come in via the emergency evaluator in our work with Terf at the moment :-) :-/ :-O ).

So if it is possible to avoid parsing at all I would go that route because, even if the code wasn’t initially as elegant (because I’d need new shout support code and an interconnection with the ContextInspector to make it read well) I’d go that route because I’d want the debugger to be as minimal as possible.  And yes, we’re using shout to style the method text itself.  But that screams to me: why aren’t we using a publish/subscribe scheme to allow the styled method to be pushed to the inspectors in the debugger do they can reuse its result?

The thing to think about fundamentally is that the debugger is a core piece of system engineering.  You’ve made it much better, but because of the importance of the tool, it’s incumbent upon you to engineer it as well as possible and not just leave it as elegant as possible.  You’ve got to also make it as robust and minimal as possible.  You will thank yourself for the extra effort at some time in the future.

And the Pharo experience?  A debugger that jams up mysteriously.  Inspectors that retain objects, causing garbage to accumulate, even after the inspectors have been closed.  These are evils to be banished with as much effort as you can muster.  

Does my perspective make sense or does it feel wrong to you?

Apart from that, to answer your questions:


As a rule of thumb, every field title should be styled exactly as if it would be typed into the evaluate/field pane - with the exception of derived fields as you mentioned, see InspectorField >> #emphasizeName. So:


- is field name emphasis expected to respond to shadowing by temp vars or not?


Yes.


> - what categories of field name occur in the debugger’s receiver inspector


This is a "regular", i.e. arbitrary inspector again, only. If you debug code in a collection, you will see a collection inspector ... if you debug code in a Morph, you will see a MorphInspector etc. ... See the whole inspector hierarchy and implementors of #inspectorClass. I have the impression that optimizing this on the level you are proposing would be a quite excessive optimization. :-)


Hmm, I see.  This is a major change in semantics, and IMO in general not a good one.  Inside a method one is seeing the raw representation of an object.  The receiver inspector used to present the view that accorded with the method, showing inside the object.  I can u set stand why one might want the external view but at the very least this should be a preference.  Ideally one would be able to toggle the preference on an individual debugger.  But, for example, if I’m debugging some internals in Dictionary et al I *need* to see that internal state in the debugger.

Note that if I want to see the external state I can always spawn a normal inspector on self.  I can’t do the opposite easily; I have to send basicInspect.

Now, if your intent is that the emphasizing of the field names reflects their emphasis in code that’s another strong argument for showing in the inspector the internal inst var names and not synthetic fields as presented by a higher level inspector.

You might experiment with a check box in the debugger control bar or just above or below the receiver inspector to allow toggling between basic and higher level inspectors. 

Please note that, for the long term, I also had in mind to fix the limitations of LazyListMorph which currently only respects the attributes of the first character of an item, and then to style custom field titles according to their code expressions. At least in this scenario, we would need to do a full Shout run indeed for the relevant fields.


Best,

Christoph


Von: Eliot Miranda <[hidden email]>
Gesendet: Samstag, 3. Oktober 2020 00:08:58
An: Thiede, Christoph
Betreff: Re: [squeak-dev] The Trunk: Tools-eem.994.mcz
 
Hi Christoph, Hi Marcel,

On Oct 2, 2020, at 2:18 PM, Thiede, Christoph <[hidden email]> wrote:



Hi Eliot,


thanks for the exact steps to reproduce. I will use [1000 timesRepeat: [self performAction]] timeProfile because I don't have the AndreasSystemProfiler.


Does ShoutCore-ct.78 from the inbox help you to speed things up? For me, it speeds up the call from 3.84 sec down to 1.07 sec - but still, there happens a lot of redundant shout styling.

Hm, why can't we recreate the fields lazily in #fields instead of doing this eagerly in #resetFields? This is how I originally implemented it but then Marcel changed it - I don't know the reason. :-)


Well, what I want to know are what are the semantics?  What I mean is that AFAIA, in an Inspector the only kinds of field names we have are:
- the pseudo variable self
- instance variable names
- integer indices (indexed inst vars)
- literal and object print strings (the keys in dictionary)
- some derived field name (such as ‘hex’ or ‘octal’ in an Integer inspector)

But in the basic object inspector in the debugger we only have the first three (I think; this is my preconception).  But Marcel, you may have something richer in mind which I may be missing.

Now, let’s assume for the moment that I’m right, then the only decisions to be made in styling the inst vars are
a) is the field name the pseudo variable self? => style as a pseudo variable
b) is it a variable name => check if it is shadowed by a temp var (which we can find out much more directly *and* correctly by asking the ContextInspector, more accurately because the ContextInspector knows which temps are in scope at the current instance)
    - if if is shadowed emphasize it red
    - if it is not shadowed emphasize it as an inst var 
c) if it is an integer emphasize it at a literal integer 

So to do this we don’t have to parse the method at all.  We only have to query Shout to find out what its emphases for particular categories are.

So if we have a better definition if the semantics (answering questions like 

     - is field name emphasis expected to respond to shadowing by temp vars or not? (I swear if autocorrect renamed var to car one more time I will scream)
     - what categories of field name occur in the debugger’s receiver inspector)

then I could optimize.  But I didn’t because I didn’t know what was going on and was afraid to break something.

Best,

Christoph


Von: Eliot Miranda <[hidden email]>
Gesendet: Freitag, 2. Oktober 2020 22:59:04
An: The general-purpose Squeak developers list; Taeumel, Marcel; Thiede, Christoph
Cc: [hidden email]
Betreff: Re: [squeak-dev] The Trunk: Tools-eem.994.mcz
 
Hi Marcel, Hi Christoph,

    I emailed the list and cc'ed you to get your attention.  Forgive my rude interruption.  I finally found out where the slow down really is.  It is in 

Inspector>>fieldList
"Return a list of texts that identify the fields for the object under inspection so that the user can make an informed decision on what to inspect."
^ self fieldListStyler
ifNil: [self fields collect: [:field | field name]]
ifNotNil: [:styler |
self updateStyler: styler.
self fields collect: [:field |
field shouldStyleName
ifTrue: [styler styledTextFor: field name asText]
ifFalse: [field name]]]

So this runs a styler over the entire method every time one steps.  And if one is stepping through a doit it will call the decompiler to generate the source to style, every time you step.  We have to do better :-)

Here's how to profile it.  Debug a doit.  I wrote this one:

| t |
t := 0.
[| a b c |
a := 1. b := 2. c := 100.
(a = 1 and: [b = 2 and: [c = 100]])
ifTrue:
[1 to: 100 by: 2 do:
[:i| t := t + 1]]
ifFalse:
[a to: c by: b do:
[:i| t := t + 1]]] repeat.
t

Once in the debugger inspect the "Over" button.  Then in that inspector evaluate 

      AndreasSystemProfiler spyOn: [1000 timesRepeat: [self performAction]]

and you'll see that essentially all the time is going into SHTextStylerST80(SHTextStyler) styledTextFor:

On Fri, Oct 2, 2020 at 1:44 PM <[hidden email]> wrote:
Eliot Miranda uploaded a new version of Tools to project The Trunk:
http://source.squeak.org/trunk/Tools-eem.994.mcz

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

Name: Tools-eem.994
Author: eem
Time: 2 October 2020, 1:44:29.015648 pm
UUID: 23145257-280d-4a4a-a4ba-3b5cc367f9a9
Ancestors: Tools-eem.993

Oops! Compiledmethod>>blockExtentsToTempsMap needs to observe that startKeysToBlockExtents has moved to DebuggerMethodMap.

=============== Diff against Tools-eem.993 ===============

Item was changed:
  ----- Method: CompiledMethod>>blockExtentsToTempsMap (in category '*Tools-Debugger-support') -----
  blockExtentsToTempsMap
        "If the receiver has been copied with temp names answer a
         map from blockExtent to temps map in the same format as
         BytecodeEncoder>>blockExtentsToTempNamesMap.  if the
         receiver has not been copied with temps answer nil."
        ^self holdsTempNames ifTrue:
+               [self mapFromBlockKeys: (self debuggerMap startKeysToBlockExtents values sort: [:assocA :assocB| assocA first <= assocB first])
-               [self mapFromBlockKeys: (self startKeysToBlockExtents values sort: [:assocA :assocB| assocA first <= assocB first])
                        toSchematicTemps: self tempNamesString]!




--
_,,,^..^,,,_
best, Eliot


Carpe Squeak!
Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Tools-eem.994.mcz

timrowledge

So, we are debating debuggers here, along with a bit about inspectors.

I'd urge keeping the 'normal' case for both as simple as possible - no styling, no trying to make special views for each object that becomes the receiver, nothing like that. Focus on the operation of the tool. Stepping needs to be fast and most importantly, correct. Rely on as little as possible because the debugger is *what has to work when you've broken everything*.

If you want fancy extras, make them extras, and make them easy to open or activate or indeed close. Make the receiver & context inspectors the most basic kind with a button to swap them for a fancier one. We already have the 'inspect/explore' switch for inspectors, for example.

Fancy highlighting and colouring, or showing tiny thumbnails of images have no place in a lot of important debugging scenarios. They are certainly an obstruction when it is the thumbnail code or styling code that has to be debugged!

More generally it would be really nice if the styling stuff could be turned off/on easily. I tolerate it on recent Pi systems but only because it is a pain to faff around removing the package.

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Strange OpCodes: IBLU: Ignore Basic Laws of Universe