Improving code


#1

It should be fairly obvious as to what I’m doing (getting information
about a process – right now just cpu percentage).

Any ideas on how I can improve this code? I’m not a big fan of how
the scanf stuff works.

This is also heavily *nix-biased, and I’m not sure how to properly
work with Windows.

require ‘scanf’

class ProcessInfo

def initialize pid
@pid = pid
end

def cpu_percentage
# if we don’t know the previous time, return 0.0
if @previous_cpu_time.nil? || @previous_time.nil?
@previous_cpu_time = get_process_cpu_time
@previous_time = Time.now
0.0
else
# Get current and elapsed time
current_time = Time.now
elapsed_time = current_time - @previous_time

  # Get current and elapsed cpu time
  current_cpu_time = get_process_cpu_time
  elapsed_cpu_time = current_cpu_time - @previous_cpu_time

  # Calculate the percentage used
  percentage = elapsed_cpu_time / elapsed_time

  # Save the current time
  @previous_time = current_time
  @previous_cpu_time = current_cpu_time

  percentage
end

end

private

Need to get the process stats

def get_process_cpu_time
stats = get_process_stats
stats[:system_cpu_time] + stats[:user_cpu_time]
end

See ‘man proc’ for details.

def get_process_stats
proc_stats = File.read("/proc/#{ @pid }/stat")

# EEWWWW -- gets the stuff from /proc/<pid>/stat and puts it into 

vars
pid, comm, state, ppid, pgrp, session, tty_nr, tpgid, flags,
minflt, cminflt, majflt, cmajflt, utime, stime, cutime,
cstime, priority, nice, who_cares, itrealvade,
starttime, vsize, rss, rlim, startcode, endcode, startstack,
kstkesp, kstkeip, signal, blocked, sigignore, sigcatch,
wchan, nswap, cnswap, exit_signal, processor =
proc_stats.scanf("%d %s %c %d %d %d %d %d %d %d
%d %d %d %d %d %d %d %d %d %d %d %d %d %d %d %d %d %d %d
%d %d %d %d %d %d %d %d %d %d %d %d %d %d %d %d")

# Just put the ones I need into the hash
stats = {}
stats[:system_cpu_time] = stime
stats[:user_cpu_time] = utime
stats[:processor] = processor
stats

end
end

example usage

fail “hey, specify a process id please!” if ARGV.size != 1
pid = ARGV.shift
c = ProcessInfo.new pid

loop do
puts “current cpu percentage = #{ c.cpu_percentage }”
sleep 3
end


#2

On 11/16/05, Joe Van D. removed_email_address@domain.invalid wrote:

def cpu_percentage
# if we don’t know the previous time, return 0.0
if @previous_cpu_time.nil? || @previous_time.nil?
@previous_cpu_time = get_process_cpu_time
@previous_time = Time.now

@previus_cpu_time ||= get_process_cpu_time
@previous_time ||= Time.now

That drops the conditional.


#3

On 11/16/05, Joe Van D. removed_email_address@domain.invalid wrote:

  wchan, nswap, cnswap, exit_signal, processor =
proc_stats.scanf("%d %s %c %d %d %d %d %d %d %d \

%d %d %d %d %d %d %d %d %d %d %d %d %d %d %d %d %d %d %d
%d %d %d %d %d %d %d %d %d %d %d %d %d %d %d %d")

proc_stats = File.read("/proc/#{ @pid }/stat").split(" ")

you might need to change your split to match the appropriate pattern,
matching a space above. This makes proc_stats an array

# Just put the ones I need into the hash
stats = {}
stats[:system_cpu_time] = stime
stats[:user_cpu_time] = utime
stats[:processor] = processor
stats

end
end

stats = { :system_cpu_time => proc_stats[14],
:user_cpu_time => proc_stats[13],
:processor => proc_stats.last }

the indicies might not be correct above, but hopefully the idea works :wink:


#4

On 11/16/05, Gregory B. removed_email_address@domain.invalid wrote:

  starttime, vsize, rss, rlim, startcode, endcode, startstack,
           :user_cpu_time     => proc_stats[13],
           :processor            => proc_stats.last }

the indicies might not be correct above, but hopefully the idea works :wink:

That is exactly what I had before I decided to use scanf. scanf has
the advantage of automatically converting everything to the correct
type. But I’m not sure what’s best in this case.

Later on, I might need to get more of the process information, so I
thought getting all the values, and then just putting the ones I need
into the hash would be better.

Which version would people rather work with?


#5

On 11/16/05, Gregory B. removed_email_address@domain.invalid wrote:

That drops the conditional.

That doesn’t return 0.0 if the program doesn’t know @previous_cpu_time
or @previous time.

I guess I could’ve done:
return 0.0 if @previous_cpu_time.nil? or @previous_time.nil?
@previous_cpu_time ||= get_process_cpu_time
@previous_time ||= Time.now


#6

On 11/17/05, Joe Van D. removed_email_address@domain.invalid wrote:

I guess I could’ve done:
return 0.0 if @previous_cpu_time.nil? or @previous_time.nil?
@previous_cpu_time ||= get_process_cpu_time
@previous_time ||= Time.now

Yes, I like that.


#7

Joe Van D. wrote:

It should be fairly obvious as to what I’m doing (getting information
about a process – right now just cpu percentage).

Any ideas on how I can improve this code? I’m not a big fan of how
the scanf stuff works.

This is also heavily *nix-biased, and I’m not sure how to properly
work with Windows.

Did you consider using Benchmark? I don’t know what you’re up to
eventually but it might well do what you are looking for.

Kind regards

robert

#8

Hi –

On Thu, 17 Nov 2005, Joe Van D. wrote:

It should be fairly obvious as to what I’m doing (getting information
about a process – right now just cpu percentage).

Any ideas on how I can improve this code? I’m not a big fan of how
the scanf stuff works.

Although I feel a parental fondness for scanf.rb, I have to say in
this case you’d probably be better off doing a split and doing the
conversions explicitly. Or you could split, join the ones you want
into a string, and then scanf that.

David


#9

On 11/17/05, Joe Van D. removed_email_address@domain.invalid wrote:

That is exactly what I had before I decided to use scanf. scanf has
the advantage of automatically converting everything to the correct
type. But I’m not sure what’s best in this case.

You could easily call to_f before you assign it to the hash.

Later on, I might need to get more of the process information, so I
thought getting all the values, and then just putting the ones I need
into the hash would be better.

you still are getting all the values. You could make an array of keys
or a temporary hash if you want to make it more clear, though…


#10

joevandyk wrote: (in part)

else
  # Get current and elapsed time
  current_time = Time.now
  elapsed_time = current_time - @previous_time

  # Get current and elapsed cpu time
  current_cpu_time = get_process_cpu_time
  elapsed_cpu_time = current_cpu_time - @previous_cpu_time

Do you want the elapsed cpu time to include the time it takes to compute
the elapsed time? You can rearrange it just a bit –
current_time = Time.now
current_cpu_time = get_process_cpu_time
elapsed_time = current_time - @previous_time
elapsed_cpu_time = current_cpu_time - @previous_cpu_time

– but that’s definitely a nit!