The Trunk: Tools-eem.994.mcz
Locked
 
|
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]! |
Re: 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: _,,,^..^,,,_ best, Eliot |
Re: The Trunk: Tools-eem.994.mcz
|
|
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: _,,,^..^,,,_
best, Eliot
Carpe Squeak!
|
|
|
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? |
Re: The Trunk: Tools-eem.994.mcz
|
|
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!
|
Re: The Trunk: Tools-eem.994.mcz
|
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
|
Re: The Trunk: Tools-eem.994.mcz
|
|
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:
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?
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.
Carpe Squeak!
|
|
|
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 |
«
Return to Squeak - Dev
|
1 view|%1 views
| Free forum by Nabble | Edit this page |