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.

What's the encoding of the filename? It's of type char_16_t*. We need a char* for the JSON. Is it UTF16?

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?

(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.

This patch is meant to be applied on top of https://bugzilla.mozilla.org/attachment.cgi?id=8364483&action=edit

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?

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.

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.

Viktor do you think we should push to try first or is it safe enough to land directly to inbound?

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?

The fun of supporting many platforms and some annoying/old compilers. Try a build of win32/mac64/linux/android and run xpcshell.

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.

Backed out for bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/6a16178f4c08 https://tbpl.mozilla.org/php/getParsedLog.php?id=34220986&tree=Mozilla-Inbound

Maybe we need to cast that to a void*?

mFilename is a const char*, and you cannot free pointers to const things. The correct fix would be to make it a char*.