[draft] top-level profiling

[mahrud]

This PR is by no means ready, but I opened it for two reasons:

1. eventually I will need help on visualizing the output of the profiler
2. I'm fairly sure I have to revert @pzinn's bf16db9 which changes positions for lots of things, so I wanted to start the discussion now.

---

Much lower priority, but for the first item above, there are several options:

1. if the output could be compatible with pprof it would be amazing, since it has a beautiful web view format, this might be hard because I think the input is in protobuf format (which is fast, very fast)
2. javascript has a simple table based output that looks pretty nice.
3. I've used Flame Graph before, but the input was very kludgy. I would prefer a better scheme.

[mahrud]

Now there's very basic tabulated profiling data:

i1 : profile(1+1)

o1 = 2

i2 : profileSummary

o2 = #run  %time   position       
     1     48.07   stdio:1:9-1:10 
     1     .47     stdio:1:8-1:9  
     1     .15     stdio:1:10-1:11
     1     .0002s  elapsed total  

The idea being that the keyword profile can be inserted in the middle of existing code (or replaced with tricks like kernel Matrix := m -> profile (lookup blah) foo), and after running it several times you can extract information using profileSummary.

[mahrud]

I'm actually more confused now. Maybe I don't need to revert all of bf16db9, but I'm confused about what is everything happening in that commit.

[mahrud]

I think I figured out what's going on. The documentation of FilePosition says:

It's implemented as a list with 3, 5 or 7 elements. The first part is the file name, then each pair is a row/column position. A single pair is a position, two form a range. The last pair is the central point of interest in that range.

But some of the methods treat the first pair as the focus point instead.

[pzinn]

which methods treat the first pair as the focus point? they shouldn't. By now the documentation is slightly obsolete in the sense that 3 elements should never occur.

[pzinn]

I'm not aware of this profiler so cannot judge how it is affected by the changes to positioning. I can only reiterate that now all code locations are made of three pieces (though the last is not displayed at the m2 level, and so one could in theory leave it out): starting position, end position, and "focus" position. The latter is at the moment only used as the location in the error message should an error occur.

[mahrud]

starting position, end position, and "focus" position. The latter is at the moment only used as the location in the error message should an error occur.

Based on my reading of your additions, you were treating the first pair as the focus point. See for instance here in locate(Position), where p.line and p.column are the first pair in p but printed last:

M2/M2/Macaulay2/d/debugging.dd

Lines 27 to 29 in b59eb20

	toExpr(int(p.line1)),toExpr(int(p.column1)),
	toExpr(int(p.line2)),toExpr(int(p.column2)),
	toExpr(int(p.line)),toExpr(int(p.column))),

I'll fix this in a PR. I think it resolves most of the issues I'm seeing, but I'm also wondering about your motivation behind unseq. My understanding is that given something like if ( true ) then ... you want the location of the predicate to be at "t" rather than "(", and I want to know if this was just a choice or if it matters for a specific error printing scenario.

[pzinn]

sorry I should've been clearer on what I meant by first or last:

export Position := {filename:string, line:ushort, column:ushort, line1:ushort, column1:ushort, line2:ushort, column2:ushort, loadDepth:ushort};

in this definition the first location is the focus location. Unfortunately it becomes the last location at the m2 level, hence the confusion. but I'm assuming you figured this out.

[pzinn]

ah yes, unseq. this is slightly annoying but it does take care of getting the parentheses included in the location. so I think it's the opposite of what you're saying.

[mahrud]

it does take care of getting the parentheses included in the location

Why do you care about including the parenthesis in the location, if it's unnecessary anyway? Seems more efficient to optimize them away as soon as possible.

[pzinn]

parentheses are usually there for a reason, and we should respect that. in the expression

(

1

)

the location should be the 5 lines, not just the one in the middle.

[mahrud]

parentheses are usually there for a reason, and we should respect that. in the expression ... the location should be the 5 lines, not just the one in the middle.

What are examples of good reasons for parentheses like this?

[pzinn]

parentheses are usually there for a reason, and we should respect that. in the expression ... the location should be the 5 lines, not just the one in the middle.

What are examples of good reasons for parentheses like this?

A lot of people will use parentheses like this, say to define a function. The whole purpose of my PR was to once and for all get the locations exactly right, instead of approximately right with lots of hacks (like randomly adding +1 to the final location to get that potential extra parenthesis).

[mahrud]

A lot of people will use parentheses like this, say to define a function.

So to be precise, is this an explicit example of what you have in mind?

f = (a,b,c) -> (

    a+b+c

    )

In this case, the previous behavior cut out the closing parenthesis, and I agree with you that this is a good fix. But what about other instances of Code in convertr.d? Say in the following ifCode

if x then (

    error 0

    )

I would want the error position to point directly to error 0 (which was the case and still is the case), not the whole enclosing parenthesis. Is there a benefit to keeping the location of the parenthesis outside of functionCode?

[pzinn]

yes, there's a benefit: otherwise the location of the if ... then ... statement will be missing its closing parenthesis.

[mahrud]

yes, there's a benefit: otherwise the location of the if ... then ... statement will be missing its closing parenthesis.

How do you get the location of an if .. then ..? (I know how to do this using pseudocode and going deep in, but would anyone do this?)

[pzinn]

it's a matter of principle. and yes, either using pseudocode, or using the debugger.