Closed Bug 851748 Opened 11 years ago Closed 11 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: 11 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.