[squeak-dev] The Trunk: Tools-eem.994.mcz

Thiede, Christoph Christoph.Thiede at student.hpi.uni-potsdam.de
Tue Oct 6 08:20:01 UTC 2020


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 <eliot.miranda at gmail.com>
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 <Christoph.Thiede at student.hpi.uni-potsdam.de> 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

<http://www.hpi.de/>
________________________________
Von: Eliot Miranda <eliot.miranda at gmail.com>
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 <Christoph.Thiede at student.hpi.uni-potsdam.de> 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

<http://www.hpi.de/>
________________________________
Von: Eliot Miranda <eliot.miranda at gmail.com>
Gesendet: Freitag, 2. Oktober 2020 22:59:04
An: The general-purpose Squeak developers list; Taeumel, Marcel; Thiede, Christoph
Cc: packages at lists.squeakfoundation.org
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 <commits at source.squeak.org<mailto:commits at source.squeak.org>> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20201006/70870abc/attachment.html>


More information about the Squeak-dev mailing list