Closed Bug 820048 Opened 8 years ago Closed 7 years ago

Add microsecond profiling

Categories

(Core :: Gecko Profiler, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

Attachments

(1 file, 6 obsolete files)

I plan on renumbering the units to microseconds and introducing microsecond profiling to osx only where it work reasonably well. We can then progressively roll out the feature to other backends.
Attached patch wip (obsolete) — Splinter Review
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
Attachment #690488 - Attachment is obsolete: true
try: -b do -p linux,linux64,macosx64,win32,android -u none -t none
Flags: needinfo?(bgirard)
I'll come back to this later.
Assignee: bgirard → nobody
Flags: needinfo?(bgirard)
Attached patch cleaner patch (obsolete) — Splinter Review
With this patch we support microsecond profiling in the mac/linux backend. The windows backend just ignores it for now.

The profiler does not guarantee the interval is respected. I want to continue improving the UI to display what was requested and how accurate the samples match the request interval.
Assignee: nobody → bgirard
Attachment #690506 - Attachment is obsolete: true
Attachment #720897 - Flags: review?(ehsan)
Comment on attachment 720897 [details] [diff] [review]
cleaner patch

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

::: tools/profiler/platform-macos.cc
@@ +281,5 @@
>      }
>      thread_resume(profiled_thread);
>    }
>  
> +  int intervalMicro_;

This is not needed.

::: tools/profiler/platform-win32.cc
@@ +72,5 @@
> +  {
> +    interval_ = floor(interval + 0.5);
> +    if (interval_ < 0) {
> +      interval_ = 1;
> +    }

Why this check?
Attachment #720897 - Flags: review?(ehsan) → review+
(In reply to :Ehsan Akhgari from comment #7)
> Comment on attachment 720897 [details] [diff] [review]
> cleaner patch
> 
> Review of attachment 720897 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: tools/profiler/platform-macos.cc
> @@ +281,5 @@
> >      }
> >      thread_resume(profiled_thread);
> >    }
> >  
> > +  int intervalMicro_;
> 
> This is not needed.

The name change is because by default the units are millisecond weather the type is int or double. In this place the units are explicitly microseconds. The const is just to be consistent with the change in the win32 backend.

> 
> ::: tools/profiler/platform-win32.cc
> @@ +72,5 @@
> > +  {
> > +    interval_ = floor(interval + 0.5);
> > +    if (interval_ < 0) {
> > +      interval_ = 1;
> > +    }
> 
> Why this check?

Actually this should be <= 0. I wanted to guard against the possibility of rounding down to zero and/or getting a bad value. I wanted to enforce a minimum of 1.
(In reply to comment #8)
> (In reply to :Ehsan Akhgari from comment #7)
> > Comment on attachment 720897 [details] [diff] [review]
> > cleaner patch
> > 
> > Review of attachment 720897 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: tools/profiler/platform-macos.cc
> > @@ +281,5 @@
> > >      }
> > >      thread_resume(profiled_thread);
> > >    }
> > >  
> > > +  int intervalMicro_;
> > 
> > This is not needed.
> 
> The name change is because by default the units are millisecond weather the
> type is int or double. In this place the units are explicitly microseconds. The
> const is just to be consistent with the change in the win32 backend.

OK.

> > ::: tools/profiler/platform-win32.cc
> > @@ +72,5 @@
> > > +  {
> > > +    interval_ = floor(interval + 0.5);
> > > +    if (interval_ < 0) {
> > > +      interval_ = 1;
> > > +    }
> > 
> > Why this check?
> 
> Actually this should be <= 0. I wanted to guard against the possibility of
> rounding down to zero and/or getting a bad value. I wanted to enforce a minimum
> of 1.

So why don't we have a similar check for other backends?
Attached patch cleaner patch v2 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=0a953edfd08b
Attachment #720897 - Attachment is obsolete: true
Attachment #720966 - Flags: review+
Ready to land, waiting on bug 779291 to minimize conflicts
Depends on: 779291
Attached patch rebased (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=887be9f8a7d0
Attachment #720966 - Attachment is obsolete: true
Attachment #729597 - Flags: review+
Attached patch yet another rebase (obsolete) — Splinter Review
try: -b do -p all -u xpcshell -t none
Attachment #729597 - Attachment is obsolete: true
Attachment #787694 - Flags: review+
Attachment #787694 - Attachment is obsolete: true
Attachment #787734 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/c1f4bf03306a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
I landed a trivial mingw fixup (missing math.h include):

https://hg.mozilla.org/integration/mozilla-inbound/rev/177637fe281f
You need to log in before you can comment on or make changes to this bug.