1186880 - Performance timing api in workers should output entires if preference is enabled

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Francisco Jordano [:arcturus] [:francisco]]

The performance timing api in content output any mark or measure if the preference 'dom.performance.enable_user_timing_logging' is enabled.

This is used in some tools, like raptor, to extract and accumulate information.

In workers we should be able to output to the console the entries information too, so we can benefit from those performance tools for workers too.

Comment 1

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: v1.diff

Hei Baku,

I'm not a gecko person ;), but tried my best, please could you guide me towards having this ready?

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 8637877 [details] [diff] [review]
v1.diff

Review of attachment 8637877 [details] [diff] [review]:
-----------------------------------------------------------------

looks good to me.

::: dom/workers/Performance.cpp
@@ +22,5 @@
>  Performance::Performance(WorkerPrivate* aWorkerPrivate)
>    : mWorkerPrivate(aWorkerPrivate)
>  {
>    mWorkerPrivate->AssertIsOnWorkerThread();
> +

remove this empty line.

@@ +71,5 @@
> +void
> +Performance::InsertUserEntry(PerformanceEntry* aEntry)
> +{
> +  if (mWorkerPrivate->PerformanceLoggingEnabled()) {
> +    PERFLOG("Performance Entry: %s|%s|%s|%f|%f|%" PRIu64 "\n",

Can you share this code with the main-thread code?
Something like:

void Performance::LogEntry(PerformanceEntry* aEntry, const nsAString& aScript);

Comment 3

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: v2.diff

Suggestions added, ready for first r?

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8638176 [details] [diff] [review]
v2.diff

Review of attachment 8638176 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsPerformance.cpp
@@ +743,5 @@
>      if(NS_FAILED(rv)) {
>        // If we have no URI, just put in "none".
>        uri.AssignLiteral("none");
>      }
> +    PerformanceBase::LogEntry(aEntry, NS_ConvertUTF8toUTF16(uri));

Just pass uri in utf8.

@@ +973,5 @@
>    ClearUserEntries(aName, NS_LITERAL_STRING("measure"));
>  }
>  
>  void
> +PerformanceBase::LogEntry(PerformanceEntry* aEntry, const nsAString& aOwner)

const nsACString& aOwner

@@ +976,5 @@
>  void
> +PerformanceBase::LogEntry(PerformanceEntry* aEntry, const nsAString& aOwner)
> +{
> +  PERFLOG("Performance Entry: %s|%s|%s|%f|%f|%" PRIu64 "\n",
> +            NS_ConvertUTF16toUTF8(aOwner).get(),

No additional converts.

::: dom/base/nsPerformance.h
@@ +349,5 @@
>    {
>      return mResourceEntries.Length() >= mResourceTimingBufferSize;
>    }
>  
> +  void LogEntry(PerformanceEntry* aEntry, const nsAString& aOwner);

void LogEntry(PerformanceEntry* aEntry, const nsACString& aOWner) const;

::: dom/workers/Performance.cpp
@@ +62,5 @@
> +void
> +Performance::InsertUserEntry(PerformanceEntry* aEntry)
> +{
> +  if (mWorkerPrivate->PerformanceLoggingEnabled()) {
> +    PerformanceBase::LogEntry(aEntry, mWorkerPrivate->ScriptURL());

NS_ConvertUTF16toUTF8(mWorkerPrivate->ScriptURL()

Comment 5

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: v3.diff

v3 with latest comments, but not compiling getting the following errors:
https://pastebin.mozilla.org/8840719

Comment 6

[Andrea Marchesini [:baku]]

Comment on attachment 8639342 [details] [diff] [review]
v3.diff

Review of attachment 8639342 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsPerformance.cpp
@@ +977,5 @@
> +PerformanceBase::LogEntry(PerformanceEntry* aEntry,
> +                          const nsACString& aOwner) const
> +{
> +  PERFLOG("Performance Entry: %s|%s|%s|%f|%f|%" PRIu64 "\n",
> +            aOwner,

1. indentation
2. aOwner.get()

Comment 7

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: v4.patch

Last version, try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9528555bd2b

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8639821 [details] [diff] [review]
v4.patch

Review of attachment 8639821 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsPerformance.cpp
@@ +976,5 @@
>  void
> +PerformanceBase::LogEntry(PerformanceEntry* aEntry, const nsACString& aOwner) const
> +{
> +  PERFLOG("Performance Entry: %s|%s|%s|%f|%f|%" PRIu64 "\n",
> +            aOwner.BeginReading(),

Indentation. aOwner should be under the "

::: dom/workers/Performance.cpp
@@ +62,5 @@
> +void
> +Performance::InsertUserEntry(PerformanceEntry* aEntry)
> +{
> +  if (mWorkerPrivate->PerformanceLoggingEnabled()) {
> +    PerformanceBase::LogEntry(aEntry, NS_ConvertUTF16toUTF8(mWorkerPrivate->ScriptURL()));

80chars. Put NS_ConvertUTF.. under aEntry.

Comment 9

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: v5.patch

Done :)

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4514c88be732

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4514c88be732