Closed Bug 1080911 Opened 10 years ago Closed 10 years ago

Sort frames by samples, not duration, in the call tree

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Product:
DevTools
Component:
Performance Tools (Profiler/Timeline)
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 36

People

(Reporter: fitzgen, Assigned: christian.pommranz)

Details

(Whiteboard: [good first bug][lang=js][mentor=vporof])

Attachments

(1 file, 2 obsolete files)

Improved test cases
24.85 KB, patch
vporof
: review+
Details | Diff | Splinter Review 
Samples are the real deal.

Duration is this thing we generated from the samples, but is funky because we don't deal with harmonics, apparently.
Whiteboard: [good first bug][lang=js][mentor=vporof]
Can we get a DXR link up in here?
Attached patch bug1080911.patch (obsolete) — — Splinter Review 

    
    

    
  
Comment on attachment 8509494 [details] [diff] [review]
bug1080911.patch

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

Nice! Did you check if all the tests still pass?

        Attachment #8509494 -
        Flags: feedback+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Fix tests (obsolete)
        — 
          — Splinter Review
      

    


  
Changed the failed tests to the expected values. All tests are now passing. However, since we're now sorting by samples and not by duration, we could change the test case to better reflect that (i.e. add another .A.B.D frame in http://dxr.mozilla.org/mozilla-central/source/browser/devtools/profiler/test/browser_profiler_tree-view-03.js#76 and update the expected results). Don't know if that's desired though.

        Attachment #8509494 -
        Attachment is obsolete: true

    
    

    
  
Test improvements are always desired! :D

    
    

    
  
Christian, did you want this patch reviewed? Can you ask me for r? when you're ready?

    
    

    
  


  
Added another .A.B.D sample to the test case to make sure some tests are failing if the entries are sorted by duration rather than samples. The expected values are updated. I changed the test cases in all tree-view-0X files to stay consistent.
Nice secondary effect: Tests are now passing on l10n formats not using '.' as decimal point, too.

        Attachment #8510274 -
        Attachment is obsolete: true

        Attachment #8512616 -
        Flags: review?(vporof)

        Attachment #8512616 -
        Flags: review?(vporof) → review+

    
    

    
  
Thanks Christian!
Status: NEW → ASSIGNED

    
    

    
  
https://hg.mozilla.org/integration/fx-team/rev/baaa389d2e23
Assignee: nobody → christian.pommranz
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js][mentor=vporof] → [good first bug][lang=js][mentor=vporof][fixed-in-fx-team]

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/baaa389d2e23
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][mentor=vporof][fixed-in-fx-team] → [good first bug][lang=js][mentor=vporof]
Target Milestone: --- → Firefox 36

    
  
Product: Firefox → DevTools

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: