Closed
Bug 901022
Opened 11 years ago
Closed 10 years ago
write to MOZ_PROFILING_FILE when the tracer is stopped
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: froydnj, Unassigned)
Details
Attachments
(1 file)
3.86 KB,
patch
|
mayhemer
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•11 years ago
|
||
The documentation in VisualEventTracer.h says something about this, but either the documentation is wrong or the code used to do this once upon a time. Regardless, having this option is helpful for tracing on non-desktop devices. I'm not attached to the environment variable name; it might even be better to make it something else, since we have an "official" profiler nowadays. Maybe MOZ_TRACE_FILE?
Attachment #785082 -
Flags: review?(honzab.moz)
Comment 2•11 years ago
|
||
Comment on attachment 785082 [details] [diff] [review] write to MOZ_PROFILING_FILE when the tracer is stopped Review of attachment 785082 [details] [diff] [review]: ----------------------------------------------------------------- What do you intend to do with the log? Currently the visualizer (the extension) doesn't support (up)load of a log to present it. Do you plan to add that functionality somehow? We could have |nsIVisualEventTracerLog nsIVisualEventTracer.loadSnaphot(filepath)|. However, please keep in mind that the visualizer has probably absolutely no security checking of the input right now. ::: xpcom/base/VisualEventTracer.cpp @@ +585,5 @@ > +VisualEventTracerLog::WriteToProfilingFile() > +{ > + const char* filename = PR_GetEnv("MOZ_PROFILING_FILE"); > + if (!filename) { > + return NS_OK; Hmm.. thinking of throwing here. When user sets the env wrong (wrong name) we should indicate non-silently we didn't write anything. According the name, I think MOZ_TRACE_FILE is OK. @@ +598,5 @@ > + nsCString json; > + GetJSONString(json); > + > + int32_t bytesWritten = PR_Write(fd, json.get(), json.Length()); > + PR_Close(fd); Main thread I/O :D No, I really wouldn't bother here. Just a reminder this can be quite a big! ::: xpcom/base/nsIVisualEventTracer.idl @@ +12,5 @@ > * is a log of various events that are monitored by a custom code > * instrumentation around the mozilla code base. > */ > > +[builtinclass, scriptable, uuid(713ee3ca-95e0-4085-8616-f6d64a9508ad)] Really needed?
Attachment #785082 -
Flags: review?(honzab.moz) → review+
Reporter | ||
Comment 3•11 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #2) > What do you intend to do with the log? Currently the visualizer (the > extension) doesn't support (up)load of a log to present it. Do you plan to > add that functionality somehow? We could have |nsIVisualEventTracerLog > nsIVisualEventTracer.loadSnaphot(filepath)|. However, please keep in mind > that the visualizer has probably absolutely no security checking of the > input right now. My modified about:timeline (https://github.com/froydnj/about-timeline) supports loading files into the viewer. It's very useful for figuring out what's happening on a not-so-capable device. > ::: xpcom/base/VisualEventTracer.cpp > @@ +585,5 @@ > > +VisualEventTracerLog::WriteToProfilingFile() > > +{ > > + const char* filename = PR_GetEnv("MOZ_PROFILING_FILE"); > > + if (!filename) { > > + return NS_OK; > > Hmm.. thinking of throwing here. When user sets the env wrong (wrong name) > we should indicate non-silently we didn't write anything. OK, I guess if we're getting called from JS, we can do that. > According the name, I think MOZ_TRACE_FILE is OK. OK, will change. > @@ +598,5 @@ > > + nsCString json; > > + GetJSONString(json); > > + > > + int32_t bytesWritten = PR_Write(fd, json.get(), json.Length()); > > + PR_Close(fd); > > Main thread I/O :D :P > ::: xpcom/base/nsIVisualEventTracer.idl > @@ +12,5 @@ > > * is a log of various events that are monitored by a custom code > > * instrumentation around the mozilla code base. > > */ > > > > +[builtinclass, scriptable, uuid(713ee3ca-95e0-4085-8616-f6d64a9508ad)] > > Really needed? Probably not entirely, but I don't think anybody is going to be implementing these in JS anytime soon. I can take them out if you like.
Comment 4•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5685f2befbf7
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in
before you can comment on or make changes to this bug.
Description
•