Make cygprofile order functions by process and thread ID.

Make cygprofile order functions by process and thread ID.

This is used to make the order of functions not depend on thread-scheduling
which can be greatly impacted when profiling is done with cygprofile.

As a result each thread has its own contiguous segment of code (ordered by
timestamp) and processes also have their code isolated (i.e. not interleaved).

BUG=372323

[pasko, 2014-06-05 11:28:48]

Flattening is not super scary (just redundant:), so I think we can commit it.
Please address my naming/style comments.

If you have someone with python readability nearby who could respond quickly, it
would be worthwhile for them to take a look.

https://codereview.chromium.org/281093002/diff/1/tools/cygprofile/mergetraces.py
File tools/cygprofile/mergetraces.py (right):

https://codereview.chromium.org/281093002/diff/1/tools/cygprofile/mergetraces...
tools/cygprofile/mergetraces.py:163: def _GetTraceArray(pid, tid):
This code operates on lists, there are no arrays.

It would be more readable to put 'trace_arrays_for_processes' as an explicit
parameter, and move it one level up in the scope.

Also, the function name is misleading, since it modifies the
'trace_arrays_for_processes'. The semantics of this function is a bit complex,
so please add a short docstring explaining the input types and what the function
does in 1-2 sentences.

https://codereview.chromium.org/281093002/diff/1/tools/cygprofile/mergetraces...
tools/cygprofile/mergetraces.py:166: found_process_index = -1
I am not a Python expert, but it seems more pythonic to use None.

https://codereview.chromium.org/281093002/diff/1/tools/cygprofile/mergetraces...
tools/cygprofile/mergetraces.py:179: process_pair = None
I would appreciate a comment hee saying something like:
# Append (pid, [(tid, [])]) and return the reference to the innermost list.

[pasko, 2014-06-05 12:16:35]

https://codereview.chromium.org/281093002/diff/1/tools/cygprofile/mergetraces.py
File tools/cygprofile/mergetraces.py (right):

https://codereview.chromium.org/281093002/diff/1/tools/cygprofile/mergetraces...
tools/cygprofile/mergetraces.py:138: def
GroupByProcessAndThreadId(sorted_trace):
I still think my function is easier to read. It passes your test.
Note, the function sorted() in python performs a stable sort.
Here we go:

  def GroupByProcessAndThreadId(input_trace):

    def Timestamp(usec, sec):
      return usec * 1000000 + sec 

    def PidAndTidFromString(pid_and_tid):
      strings = pid_and_tid.split(':')
      return (int(strings[0]), int(strings[1]))

    pid_first_seen = {}
    tid_first_seen = {}
    for (sec, usec, pid_and_tid, address) in input_trace:
      (pid, tid) = PidAndTidFromString(pid_and_tid)
      if not pid in pid_first_seen:
        pid_first_seen[pid] = Timestamp(usec, sec)
      if not tid in tid_first_seen:
        tid_first_seen[tid] = Timestamp(usec, sec)

    def CompareEvents(event1, event2):
      (sec1, usec1, pid_and_tid, address1) = event1
      (pid1, tid1) = PidAndTidFromString(pid_and_tid)
      (sec2, usec2, pid_and_tid, address2) = event2
      (pid2, tid2) = PidAndTidFromString(pid_and_tid)
      
      pid_cmp = cmp(pid_first_seen[pid1], pid_first_seen[pid2])
      if pid_cmp != 0:
        return pid_cmp
      tid_cmp = cmp(tid_first_seen[tid1], tid_first_seen[tid2])
      if tid_cmp != 0:
        return tid_cmp
      return cmp(Timestamp(usec1, sec1), Timestamp(usec2, sec2))
    
    return sorted(input_trace, cmp=CompareEvents)

[Philippe, 2014-06-05 12:38:52]

Thanks, Egor!

https://codereview.chromium.org/281093002/diff/1/tools/cygprofile/mergetraces.py
File tools/cygprofile/mergetraces.py (right):

https://codereview.chromium.org/281093002/diff/1/tools/cygprofile/mergetraces...
tools/cygprofile/mergetraces.py:138: def
GroupByProcessAndThreadId(sorted_trace):
On 2014/06/05 12:16:36, pasko wrote:
> I still think my function is easier to read. It passes your test.
> Note, the function sorted() in python performs a stable sort.
> Here we go:
> 
>   def GroupByProcessAndThreadId(input_trace):
> 
>     def Timestamp(usec, sec):
>       return usec * 1000000 + sec 
> 
>     def PidAndTidFromString(pid_and_tid):
>       strings = pid_and_tid.split(':')
>       return (int(strings[0]), int(strings[1]))
> 
>     pid_first_seen = {}
>     tid_first_seen = {}
>     for (sec, usec, pid_and_tid, address) in input_trace:
>       (pid, tid) = PidAndTidFromString(pid_and_tid)
>       if not pid in pid_first_seen:
>         pid_first_seen[pid] = Timestamp(usec, sec)
>       if not tid in tid_first_seen:
>         tid_first_seen[tid] = Timestamp(usec, sec)
> 
>     def CompareEvents(event1, event2):
>       (sec1, usec1, pid_and_tid, address1) = event1
>       (pid1, tid1) = PidAndTidFromString(pid_and_tid)
>       (sec2, usec2, pid_and_tid, address2) = event2
>       (pid2, tid2) = PidAndTidFromString(pid_and_tid)
>       
>       pid_cmp = cmp(pid_first_seen[pid1], pid_first_seen[pid2])
>       if pid_cmp != 0:
>         return pid_cmp
>       tid_cmp = cmp(tid_first_seen[tid1], tid_first_seen[tid2])
>       if tid_cmp != 0:
>         return tid_cmp
>       return cmp(Timestamp(usec1, sec1), Timestamp(usec2, sec2))
>     
>     return sorted(input_trace, cmp=CompareEvents)

Right :) I guess I had to see it to believe it. Thanks a lot for writing it!

https://codereview.chromium.org/281093002/diff/1/tools/cygprofile/mergetraces...
tools/cygprofile/mergetraces.py:163: def _GetTraceArray(pid, tid):
On 2014/06/05 11:28:49, pasko wrote:
> This code operates on lists, there are no arrays.
> 
> It would be more readable to put 'trace_arrays_for_processes' as an explicit
> parameter, and move it one level up in the scope.
> 
> Also, the function name is misleading, since it modifies the
> 'trace_arrays_for_processes'. The semantics of this function is a bit complex,
> so please add a short docstring explaining the input types and what the
function
> does in 1-2 sentences.

Done.

https://codereview.chromium.org/281093002/diff/1/tools/cygprofile/mergetraces...
tools/cygprofile/mergetraces.py:166: found_process_index = -1
On 2014/06/05 11:28:49, pasko wrote:
> I am not a Python expert, but it seems more pythonic to use None.

Done.

https://codereview.chromium.org/281093002/diff/1/tools/cygprofile/mergetraces...
tools/cygprofile/mergetraces.py:179: process_pair = None
On 2014/06/05 11:28:49, pasko wrote:
> I would appreciate a comment hee saying something like:
> # Append (pid, [(tid, [])]) and return the reference to the innermost list.

Done.

[pasko, 2014-06-05 12:50:10]

LGTM! Great! Special thanks for the new test.

https://codereview.chromium.org/281093002/diff/40001/tools/cygprofile/mergetr...
File tools/cygprofile/mergetraces.py (right):

https://codereview.chromium.org/281093002/diff/40001/tools/cygprofile/mergetr...
tools/cygprofile/mergetraces.py:221: merged_trace = None
nit: seems not necessary

[Philippe, 2014-06-05 13:00:45]

Thanks, Egor!

https://codereview.chromium.org/281093002/diff/40001/tools/cygprofile/mergetr...
File tools/cygprofile/mergetraces.py (right):

https://codereview.chromium.org/281093002/diff/40001/tools/cygprofile/mergetr...
tools/cygprofile/mergetraces.py:221: merged_trace = None
On 2014/06/05 12:50:10, pasko wrote:
> nit: seems not necessary

Done.

[pasko, 2014-06-11 12:19:19]

This was committed as r275076, not sure why the codereview issue is not closed.
Thanks infrastructure.

Closing.