Closed
Bug 962325
Opened 11 years ago
Closed 11 years ago
Add filename to profiler I/O markers
Categories
(Core :: Gecko Profiler, defect)
Core
Gecko Profiler
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: bugzilla, Assigned: vikstrous)
Details
Attachments
(1 file, 4 obsolete files)
3.22 KB,
patch
|
Details | Diff | Splinter Review |
In bug 902587 I am adding support for the IOInterposeObserver::Observation::Filename() function, which until now has just returned nullptr. Now that Filename() is going to be returning useful information, we should add that to profiler I/O markers.
This should be a pretty simple patch. The first parameter to the IOMarkerPayload constructor is for the "source" of the event. Currently that just passes in the result of IOInterposeObserver::Observation::Reference(). That just needs to be changed to accept both the reference and the filename (it would be nice to have both).
Note that Filename() can still be nullptr if the IOInterposer event source could not obtain the filename for some reason.
Assignee | ||
Comment 1•11 years ago
|
||
What's the encoding of the filename? It's of type char_16_t*. We need a char* for the JSON. Is it UTF16?
Flags: needinfo?(aklotz)
Assignee | ||
Comment 2•11 years ago
|
||
I applied the patch titled "P2B: Filename for PoisonIOInterposer" from bug 902587 when I noticed the char16_t issue. It looks like you changed the return type of IOInterposeObserver::Observation::Filename(). Is there any specific reason for this? It looks like we might end up doing two conversions (to and from char16_t*) for no reason in some cases.
Also, I tried converting the char16_t* back to char* like this but it didn't seem to work:
NS_ConvertUTF16toUTF8(aObservation.Filename()).get()
What's the right way of doing this?
Reporter | ||
Comment 3•11 years ago
|
||
(In reply to Viktor Stanchev [:vikstrous] from comment #1)
> What's the encoding of the filename? It's of type char_16_t*. We need a
> char* for the JSON. Is it UTF16?
It's UTF-16.
Note that JSObjectBuilder already has an overload for DefineProperty() that accepts nsAString, though JSCustomObjectBuilder does not. It might just be easiest to implement a similar overload for JSCustomObjectBuilder. I'd suggest discussing this with Benoit about how he wants to handle it.
(In reply to Viktor Stanchev [:vikstrous] from comment #2)
> I applied the patch titled "P2B: Filename for PoisonIOInterposer" from bug
> 902587 when I noticed the char16_t issue. It looks like you changed the
> return type of IOInterposeObserver::Observation::Filename(). Is there any
> specific reason for this? It looks like we might end up doing two
> conversions (to and from char16_t*) for no reason in some cases.
Some platforms use UTF-8 for Unicode but others *cough* Windows *cough* require UTF-16. jschars are 16 bits. Some consumers of this data will use an 8-bit encoding instead. Something will need to be converted at some point, and in certain situations on certain platforms these back-and-forth conversions are inevitable. Could we get more fancy to avoid these? Yes. But introducing additional complexities should be weighed against measurable improvements.
The profiler isn't the only consumer of this data. Telemetry is also going to be using it, and those filenames will be reflected into JS directly as UTF-16. There may be other consumers in the future. I settled on UTF-16 as the encoding to use in general.
>
> Also, I tried converting the char16_t* back to char* like this but it didn't
> seem to work:
>
> NS_ConvertUTF16toUTF8(aObservation.Filename()).get()
>
> What's the right way of doing this?
I'd need to see more context to know how you're using this. If you were to assign the result of that expression to a char*, you're going have trouble when that temporary object goes out of scope.
Flags: needinfo?(aklotz)
Assignee | ||
Comment 4•11 years ago
|
||
This patch is meant to be applied on top of https://bugzilla.mozilla.org/attachment.cgi?id=8364483&action=edit
Attachment #8367332 -
Flags: review?(aklotz)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → vstanchev
Reporter | ||
Comment 5•11 years ago
|
||
Comment on attachment 8367332 [details] [diff] [review]
Add filename information for file operation tags into the profiler json result
Review of attachment 8367332 [details] [diff] [review]:
-----------------------------------------------------------------
The code looks okay, but your diff is backwards: its additions and deletions are reversed. Can you please regenerate the diff and include author info and a commit message?
Attachment #8367332 -
Flags: review?(aklotz) → review-
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #8367332 -
Attachment is obsolete: true
Attachment #8367935 -
Flags: review?(bgirard)
Comment 7•11 years ago
|
||
Comment on attachment 8367935 [details] [diff] [review]
Add filename to profiler I/O markers
Review of attachment 8367935 [details] [diff] [review]:
-----------------------------------------------------------------
::: tools/profiler/ProfilerMarkers.h
@@ +135,4 @@
> typename Builder::Object preparePayloadImp(Builder& b);
>
> const char* mSource;
> + const char16_t* mFilename;
IMO this should be utf8. Lets convert before we construct IOMarkerPayload.
Attachment #8367935 -
Flags: review?(bgirard) → feedback+
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #8367935 -
Attachment is obsolete: true
Attachment #8368123 -
Flags: review?(bgirard)
Comment 9•11 years ago
|
||
Comment on attachment 8368123 [details] [diff] [review]
Add filename to profiler I/O markers
Review of attachment 8368123 [details] [diff] [review]:
-----------------------------------------------------------------
::: tools/profiler/ProfilerIOInterposeObserver.cpp
@@ +35,5 @@
> return;
> }
> ProfilerBacktrace* stack = profiler_get_backtrace();
> +
> + const char *filename = strdup((NS_ConvertUTF16toUTF8(aObservation.Filename())).get());
Do you mind moving the strdup to the constructor? It's easier to match the allocation/free if they both happen in the constructor/destructor.
Attachment #8368123 -
Flags: review?(bgirard) → review+
Assignee | ||
Comment 10•11 years ago
|
||
Comment 11•11 years ago
|
||
Viktor do you think we should push to try first or is it safe enough to land directly to inbound?
Flags: needinfo?(vstanchev)
Assignee | ||
Comment 12•11 years ago
|
||
I don't know. Every time I assume something is safe it breaks something, so I'd rather push to try first. What try chooser settings should I use? Which tests?
Flags: needinfo?(vstanchev)
Comment 13•11 years ago
|
||
The fun of supporting many platforms and some annoying/old compilers.
Try a build of win32/mac64/linux/android and run xpcshell.
Assignee | ||
Comment 14•11 years ago
|
||
Comment 15•11 years ago
|
||
Comment on attachment 8368123 [details] [diff] [review]
Add filename to profiler I/O markers
Review of attachment 8368123 [details] [diff] [review]:
-----------------------------------------------------------------
::: tools/profiler/ProfilerMarkers.cpp
@@ +121,5 @@
> MOZ_ASSERT(aSource);
> }
>
> +IOMarkerPayload::~IOMarkerPayload(){
> + delete mFilename;
If strdup was part of the c++ library it would be delete[] and not delete. But strdup is part of the c library so it should be free().
I'll just fix it before landing it however.
Comment 16•11 years ago
|
||
Comment 17•11 years ago
|
||
Comment 19•11 years ago
|
||
mFilename is a const char*, and you cannot free pointers to const things. The correct fix would be to make it a char*.
Assignee | ||
Comment 20•11 years ago
|
||
Attachment #8368123 -
Attachment is obsolete: true
Attachment #8369558 -
Attachment is obsolete: true
Comment 21•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Updated•11 years ago
|
Flags: needinfo?(bgirard)
You need to log in
before you can comment on or make changes to this bug.
Description
•