Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

Trunk
mozilla22
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 1 obsolete attachment)

There's a lot of trivial stuff in common between the two. The major difference is their ::Tick implementation. Let's merge the low hanging fruit off here.

Depends on bug 851611 to prevent rot.
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #725680 - Flags: review?(jseward)
Posted patch Part 2: Share SaveProfileTask (obsolete) — Splinter Review
Attachment #725707 - Flags: review?(jseward)
getpid build failure fixed
Attachment #725707 - Attachment is obsolete: true
Attachment #725707 - Flags: review?(jseward)
Attachment #725716 - Flags: review?(jseward)
This actually brings a few missing features that didn't get merge to ThreadProfile2 while the Breakpad stuff was developped so this is fairly important. Without this markers wont work correctly if using PROFILER_NEW
Attachment #725723 - Flags: review?(jseward)
Sorry for the review spam. Just figured more small patches are easier to review.
Attachment #725728 - Flags: review?(jseward)
The only real difference in TableTicker is Tick. This patch makes this obvious.
Attachment #725731 - Flags: review?(jseward)
Blocks: 851828
Attachment #725680 - Flags: review?(jseward) → review+
Comment on attachment 725716 [details] [diff] [review]
Part 2: Share SaveProfileTask

Nice cleanup.
Attachment #725716 - Flags: review?(jseward) → review+
(In reply to Julian Seward from comment #10)
Meh .. tried to add, but failed: it would be nice to move
SaveProfileTask::Run into its own cpp file instead of having
it in the header.
Comment on attachment 725723 [details] [diff] [review]
Part 3: Share ThreadProfile

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

::: tools/profiler/TableTicker.cpp
@@ -89,1 @@
>  #define PROFILE_MAX_ENTRY 100000

PROFILE_MAX_ENTRY needs to be retained, is that so?
Attachment #725723 - Flags: review?(jseward) → review+
Attachment #725728 - Flags: review?(jseward) → review+
Attachment #725731 - Flags: review?(jseward) → review+
(In reply to Julian Seward from comment #12)
> Comment on attachment 725723 [details] [diff] [review]
> Part 3: Share ThreadProfile
> 
> Review of attachment 725723 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: tools/profiler/TableTicker.cpp
> @@ -89,1 @@
> >  #define PROFILE_MAX_ENTRY 100000
> 
> PROFILE_MAX_ENTRY needs to be retained, is that so?

I don't understand. PROFILE_MAX_ENTRY is still duplicated. But this needs to land ASAP so I plan on fixing this later.
Planning to push a rollup patch:
https://tbpl.mozilla.org/?tree=Try
Flags: needinfo?(bgirard)
Wrong link:
https://tbpl.mozilla.org/?tree=Try&rev=aaab3aed7214
Flags: needinfo?(bgirard)
https://hg.mozilla.org/mozilla-central/rev/f23a97b6dea1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Depends on: 859928
You need to log in before you can comment on or make changes to this bug.