Stack Trace for ASH?

heeheehee said:
This change sometimes triggers an EmptyStackException which didn't exist before.
Click to expand...
Not having this line creates a memory leak of one CallFrame per script invocation.

I added a guard, since I'm still not sure why the EmptyStackException was happening.

More importantly, it looks like there's also a leak tied to each invocation of a built-in function. Apparently LibraryFunction invocations bypass FunctionReturn entirely; we probably shouldn't be spreading the stack tracking across both those classes anyways.

Both of those leaks should be resolved by the attached patch.
 

Attachments

Last edited:
You fixed something :-) System resource usage was reasonable, performance was acceptable and none of the tools said the heap was growing in size. The Out of Memory error did not occur. I'll try and get to some functionality testing.
 
fronobulax said:
You fixed something :-) System resource usage was reasonable, performance was acceptable and none of the tools said the heap was growing in size. The Out of Memory error did not occur. I'll try and get to some functionality testing.
Click to expand...

Yeah, it was pretty bad; I saw stack sizes of 1000+ just from my loginScript. I must not have seen it initially because of the hard limit on stack size I'd initially put in place.

There's also a smaller memory leak in my last patch in that we don't clean up large stacks fully when we print a stack trace; I'll probably post a fix for that whenever I get around to posting a new patch.
 
So I realized the next step is to write the function that prints and then link it to things. First thing I wonder about is you changed a lot of references to (static) Interpreter to the local instance. But the traceStream is still a static variable. Various analysis tools express concern about that. So is regular tracing supposed to be global across all Interpreters or can it be "local"? I'm kind of torn between my desire to clean up and my understanding that if I do so I have more testing. But if something is broken it may have happened with the patch. Did you test tracing? No problem if you didn't. I'm just lazy enough to not test something that wasn't touched.
 
I did not test tracing. I don't know enough about the purpose of tracing to form an opinion there. Although, I will note that .traceUnindent() and .isTracing() are not static methods (and ditto for probably some other methods that touch tracing).

My local copy adds support for an ASH function that exposes the stack trace fully (and adds a global pref to allow abort() and other script terminations to print a stack trace), but I'm not super happy with the implementation of either. Attached anyways; usage is something like
Code: 
foreach i, rec in get_stack_trace() {
  print(rec.name + " (" + rec.file + ":"  + rec.line  + ")");
}
 

Attachments

I take part of that back; isTracing is indeed static, although it's referenced non-statically. There's also gems like
Code: 
	public final void resetTracing()
	{
		this.traceIndentation = 0;
		this.traceStream = Interpreter.traceStream;
	}
where I'm not sure that last line does anything...
 
I'll run with your latest, mod it as I see fit, and test tracing before and after and report back eventually. Thanks.
 
ASH used to log all its execution to the DEBUG file. It was very verbose. I moved it to go to a TRACE file, since I wanted to be able to DEBUG request/response stuff without having ASH execution clogging it up - and have the ASH stuff not be clogged up by massive HTML strings. Considering that it still dumps strings, if you pass around a page of HTML to various functions, trace files are still clogged.

The trace file stream is static - like the debug file - since I didn't think multiple output streams going to the same file on disk would work well. As it is, I don't think output streams are thread safe, and since you can have multiple ASH interpreters running simultaneously in different threads, that's probably not going to work well, anyway.

Can't say I've ever used the trace file. Seemed like a good idea at the time, but it turned out to not be useful - aside from being implemented by someone who was still self-teaching Java. I am much more skilled at it, these days. ;)
 
Quick Google search suggests there is no thread safe output stream so we run the risks or roll our own. I'll look at that eventually. Thanks for the confirmation that the intent was to have all trace output from all interpreters go to one file.
 
Stack trace looks fine. Interesting to note that there are a couple scripts that do Abort and then handle the abort. That generates a trace that really isn't needed, but is not a problem since trace on abort is opt in.

Execution tracing is not doing what I expect, but that is the case without the patch. Either my expectations are wrong or it was broken some time ago. I'll fix it or propose it be ripped out :-)
 
My expectations were wrong :-)

debug trace on is not the same command as debug ash on. Having figured that out execution tracing seems to work. Everything else seems to behave so I will commit this (with my tweaks) later today unless I hear otherwise :-)
 
You must log in or register to reply here.
Back
Top