Closed Bug 851748 Opened 12 years ago Closed 12 years ago

Merge TableTicker1+2

Categories

(Core :: Gecko Profiler, defect)

Product:
Core
Component:
Gecko Profiler
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla22

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

Attachments

(5 files, 1 obsolete file)

Part 1: Merge ProfileEntry
39.76 KB, patch
jseward
: review+
Details | Diff | Splinter Review
Part 2: Share SaveProfileTask
12.49 KB, patch
jseward
: review+
Details | Diff | Splinter Review
Part 3: Share ThreadProfile
27.86 KB, patch
jseward
: review+
Details | Diff | Splinter Review
Part 4: Share 5 trivial functions
5.95 KB, patch
jseward
: review+
Details | Diff | Splinter Review
Part 5: Share TableTicker
15.19 KB, patch
jseward
: review+
Details | Diff | Splinter Review
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)
Attached 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)
Blocks: 854810
https://hg.mozilla.org/mozilla-central/rev/f23a97b6dea1
Status: ASSIGNED → RESOLVED
Closed: 12 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.

Attachment

General

Creator:
Created:
Updated:
Size: