condormatic performance improvements

[Jan Provazník]

Hi,
this patchset improves performance of syncing instances state
in condor util, so instances listing in a pool is faster.

[PATCH aggregator 1/2] Replaced REXML by Nokogiri in condormatic

[Jan Provazník]

From: Jan Provaznik

Use Nokogiri for parsing condor output improves noticeably
performance of listing instances in pool.
---
 src/app/util/condormatic.rb |   35 +++++------------------------------
 1 files changed, 5 insertions(+), 30 deletions(-)

diff --git a/src/app/util/condormatic.rb b/src/app/util/condormatic.rb
index 155fa7d..5fba43e 100644
--- a/src/app/util/condormatic.rb
+++ b/src/app/util/condormatic.rb
@@ -17,7 +17,7 @@
 # MA  02110-1301, USA.  A copy of the GNU General Public License is
 # also available at http://www.gnu.org/copyleft/gpl.html.

-require 'rexml/document'
+require 'nokogiri'

def condormatic_instance_create(task)

@@ -126,35 +126,10 @@ def condormatic_instances_sync_states
       instance.save!
     end

-    def find_value_int(job_ele, attrib)
-      if job_ele.attributes['n'] == attrib
-        cmd = job_ele.elements.each('i') do |i|
-          return i.text
-        end
-      end
-      return nil
-    end
-
-    def find_value_str(job_ele, attrib)
-      if job_ele.attributes['n'] == attrib
-        cmd = job_ele.elements.each('s') do |s|
-          return s.text
-        end
-      end
-      return nil
-    end
-
-    doc = REXML::Document.new(xml)
-    doc.elements.each('classads/c') do |jobs_ele|
-      job_name = nil
-      job_state = nil
-
-      jobs_ele.elements.each('a') do |job_ele|
-        value = find_value_str(job_ele, 'Cmd')
-        job_name = value if value != nil
-        value = find_value_int(job_ele, 'JobStatus')
-        job_state = value if value != nil
-      end
+    doc = Nokogiri::XML(xml)
+    doc.xpath('/classads/c').each do |jobs_ele|
+      job_name = (v = jobs_ele.at_xpath('./a[@n="Cmd"]/s')) ? v.text : nil
+      job_state= (v = jobs_ele.at_xpath('./a[@n="JobStatus"]/i')) ? v.text : nil

Rails.logger.info "job name is #{job_name}"
       Rails.logger.info "job state is #{job_state}"

[PATCH aggregator 1/2] Replaced REXML by Nokogiri in condormatic

[Ian Main]

On Wed, 2010-07-21 at 16:21 +0200, [e-mail] wrote:
> From: Jan Provaznik
>
> Use Nokogiri for parsing condor output improves noticeably
> performance of listing instances in pool.
>
> src/app/util/condormatic.rb |   35 +++++------------------------------
>  1 files changed, 5 insertions(+), 30 deletions(-)
>
> diff --git a/src/app/util/condormatic.rb b/src/app/util/condormatic.rb
> index 155fa7d..5fba43e 100644
> --- a/src/app/util/condormatic.rb
> +++ b/src/app/util/condormatic.rb
> @@ -17,7 +17,7 @@
>  # MA  02110-1301, USA.  A copy of the GNU General Public License is
>  # also available at http://www.gnu.org/copyleft/gpl.html.
>
> -require 'rexml/document'
> +require 'nokogiri'
>
> def condormatic_instance_create(task)
>
> @@ -126,35 +126,10 @@ def condormatic_instances_sync_states
>        instance.save!
>      end
> def find_value_int(job_ele, attrib)
>  if job_ele.attributes['n'] == attrib
>    cmd = job_ele.elements.each('i') do |i|
>      return i.text
>    end
>  end
>  return nil
> end
> def find_value_str(job_ele, attrib)
>  if job_ele.attributes['n'] == attrib
>    cmd = job_ele.elements.each('s') do |s|
>      return s.text
>    end
>  end
>  return nil
> end
> doc = REXML::Document.new(xml)
> doc.elements.each('classads/c') do |jobs_ele|
>  job_name = nil
>  job_state = nil
>  jobs_ele.elements.each('a') do |job_ele|
>    value = find_value_str(job_ele, 'Cmd')
>    job_name = value if value != nil
>    value = find_value_int(job_ele, 'JobStatus')
>    job_state = value if value != nil
>  end
>
> doc = Nokogiri::XML(xml)
>
> doc.xpath('/classads/c').each do |jobs_ele|
>  job_name = (v = jobs_ele.at_xpath('./a[@n="Cmd"]/s')) ? v.text : nil
>  job_state= (v = jobs_ele.at_xpath('./a[@n="JobStatus"]/i')) ? v.text : nil
>
>  Rails.logger.info "job name is #{job_name}"
>  Rails.logger.info "job state is #{job_state}"

Wow, it looks like perl^H^H^H^H line noise! ;-)

Still good though :)

ACK

[PATCH aggregator 1/2] Replaced REXML by Nokogiri in condormatic

[Jason Guiditta]

Ian is further reviewing this, but minor nit on this inline.

On Wed, 2010-07-21 at 16:21 +0200, [e-mail] wrote:
> From: Jan Provaznik
>
> Use Nokogiri for parsing condor output improves noticeably
> performance of listing instances in pool.
>
> src/app/util/condormatic.rb |   35 +++++------------------------------
>  1 files changed, 5 insertions(+), 30 deletions(-)
>
> diff --git a/src/app/util/condormatic.rb b/src/app/util/condormatic.rb
> index 155fa7d..5fba43e 100644
> --- a/src/app/util/condormatic.rb
> +++ b/src/app/util/condormatic.rb
> @@ -17,7 +17,7 @@
>  # MA  02110-1301, USA.  A copy of the GNU General Public License is
>  # also available at http://www.gnu.org/copyleft/gpl.html.
>
> -require 'rexml/document'
> +require 'nokogiri'
>
> def condormatic_instance_create(task)
>
> @@ -126,35 +126,10 @@ def condormatic_instances_sync_states
>        instance.save!
>      end
> def find_value_int(job_ele, attrib)
>  if job_ele.attributes['n'] == attrib
>    cmd = job_ele.elements.each('i') do |i|
>      return i.text
>    end
>  end
>  return nil
> end
> def find_value_str(job_ele, attrib)
>  if job_ele.attributes['n'] == attrib
>    cmd = job_ele.elements.each('s') do |s|
>      return s.text
>    end
>  end
>  return nil
> end
> doc = REXML::Document.new(xml)
> doc.elements.each('classads/c') do |jobs_ele|
>  job_name = nil
>  job_state = nil
>  jobs_ele.elements.each('a') do |job_ele|
>    value = find_value_str(job_ele, 'Cmd')
>    job_name = value if value != nil
>    value = find_value_int(job_ele, 'JobStatus')
>    job_state = value if value != nil
>  end
> doc = Nokogiri::XML(xml)
> doc.xpath('/classads/c').each do |jobs_ele|

I know you were just following the convention from the old version here,
but I think the code would be clearer if you changed all instances of
'jobs_ele' to 'job'.
>  job_name = (v = jobs_ele.at_xpath('./a[@n="Cmd"]/s')) ? v.text : nil
>  job_state= (v = jobs_ele.at_xpath('./a[@n="JobStatus"]/i')) ? v.text : nil
>
>  Rails.logger.info "job name is #{job_name}"
>  Rails.logger.info "job state is #{job_state}"

[PATCH aggregator 1/2] Replaced REXML by Nokogiri in condormatic

[Ian Main]

On Fri, 2010-07-23 at 16:28 -0400, Jason Guiditta wrote:
> Ian is further reviewing this, but minor nit on this inline.
>
> On Wed, 2010-07-21 at 16:21 +0200, [e-mail] wrote:
> > From: Jan Provaznik
> >
> > Use Nokogiri for parsing condor output improves noticeably
> > performance of listing instances in pool.
> >
> > src/app/util/condormatic.rb |   35 +++++------------------------------
> >  1 files changed, 5 insertions(+), 30 deletions(-)
> >
> > diff --git a/src/app/util/condormatic.rb b/src/app/util/condormatic.rb
> > index 155fa7d..5fba43e 100644
> > --- a/src/app/util/condormatic.rb
> > +++ b/src/app/util/condormatic.rb
> > @@ -17,7 +17,7 @@
> >  # MA  02110-1301, USA.  A copy of the GNU General Public License is
> >  # also available at http://www.gnu.org/copyleft/gpl.html.
> >
> > -require 'rexml/document'
> > +require 'nokogiri'
> >
> > def condormatic_instance_create(task)
> >
> > @@ -126,35 +126,10 @@ def condormatic_instances_sync_states
> >        instance.save!
> >      end
> > def find_value_int(job_ele, attrib)
> >  if job_ele.attributes['n'] == attrib
> >    cmd = job_ele.elements.each('i') do |i|
> >      return i.text
> >    end
> >  end
> >  return nil
> > end
> > def find_value_str(job_ele, attrib)
> >  if job_ele.attributes['n'] == attrib
> >    cmd = job_ele.elements.each('s') do |s|
> >      return s.text
> >    end
> >  end
> >  return nil
> > end
> > doc = REXML::Document.new(xml)
> > doc.elements.each('classads/c') do |jobs_ele|
> >  job_name = nil
> >  job_state = nil
> >  jobs_ele.elements.each('a') do |job_ele|
> >    value = find_value_str(job_ele, 'Cmd')
> >    job_name = value if value != nil
> >    value = find_value_int(job_ele, 'JobStatus')
> >    job_state = value if value != nil
> >  end
> > doc = Nokogiri::XML(xml)
> > doc.xpath('/classads/c').each do |jobs_ele|
>
> I know you were just following the convention from the old version here,
> but I think the code would be clearer if you changed all instances of
> 'jobs_ele' to 'job'.

Agreed.

Ian
> >  job_name = (v = jobs_ele.at_xpath('./a[@n="Cmd"]/s')) ? v.text : nil
> >  job_state= (v = jobs_ele.at_xpath('./a[@n="JobStatus"]/i')) ? v.text : nil
> >
> >  Rails.logger.info "job name is #{job_name}"
> >  Rails.logger.info "job state is #{job_state}"
>
> deltacloud-devel mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/deltacloud-devel

[PATCH aggregator 2/2] Condormatic small performance improvement

[Jan Provazník]

From: Jan Provaznik

Instance state in condormatic_instances_sync_states is set
only once.
---
 src/app/util/condormatic.rb |   21 +++++++--------------
 1 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/src/app/util/condormatic.rb b/src/app/util/condormatic.rb
index 5fba43e..3a21d64 100644
--- a/src/app/util/condormatic.rb
+++ b/src/app/util/condormatic.rb
@@ -118,15 +118,8 @@ def condormatic_instances_sync_states

raise ("Error calling condor_q -xml") if $? != 0

-    # Set them all to 'stopped' because if they aren't in the condor
-    # queue as jobs then they are not running, pending or anything else.
-    instances = Instance.find(:all)
-    instances.each do |instance|
-      instance.state = Instance::STATE_STOPPED
-      instance.save!
-    end
-
     doc = Nokogiri::XML(xml)
+    jobs_state = {}
     doc.xpath('/classads/c').each do |jobs_ele|
       job_name = (v = jobs_ele.at_xpath('./a[@n="Cmd"]/s')) ? v.text : nil
       job_state= (v = jobs_ele.at_xpath('./a[@n="JobStatus"]/i')) ? v.text : nil
@@ -134,13 +127,13 @@ def condormatic_instances_sync_states
       Rails.logger.info "job name is #{job_name}"
       Rails.logger.info "job state is #{job_state}"

-      instance = Instance.find(:first, :conditions => {:condor_job_id => job_name})
+      jobs_state[job_name] = condor_to_instance_state(job_state) if job_name
+    end

-      if instance
-        instance.state = condor_to_instance_state(job_state)
-        instance.save!
-        Rails.logger.info "Instance state updated to #{condor_to_instance_state(job_state)}"
-      end
+    Instance.find(:all).each do |instance|
+      instance.state = jobs_state[instance.condor_job_id] || Instance::STATE_STOPPED
+      instance.save!
+      Rails.logger.info "Instance state updated to #{instance.state}"
     end
   rescue Exception => ex
     Rails.logger.error ex.message

[PATCH aggregator 2/2] Condormatic small performance improvement

[Ian Main]

On Wed, 2010-07-21 at 16:21 +0200, [e-mail] wrote:
> From: Jan Provaznik
>
> Instance state in condormatic_instances_sync_states is set
> only once.
>
> src/app/util/condormatic.rb |   21 +++++++--------------
>  1 files changed, 7 insertions(+), 14 deletions(-)
>
> diff --git a/src/app/util/condormatic.rb b/src/app/util/condormatic.rb
> index 5fba43e..3a21d64 100644
> --- a/src/app/util/condormatic.rb
> +++ b/src/app/util/condormatic.rb
> @@ -118,15 +118,8 @@ def condormatic_instances_sync_states
>  raise ("Error calling condor_q -xml") if $? != 0
>
> # Set them all to 'stopped' because if they aren't in the condor
> # queue as jobs then they are not running, pending or anything else.
> instances = Instance.find(:all)
> instances.each do |instance|
>  instance.state = Instance::STATE_STOPPED
>  instance.save!
> end
> doc = Nokogiri::XML(xml)
> jobs_state = {}
> doc.xpath('/classads/c').each do |jobs_ele|
>   job_name = (v = jobs_ele.at_xpath('./a[@n="Cmd"]/s')) ? v.text : nil
>   job_state= (v = jobs_ele.at_xpath('./a[@n="JobStatus"]/i')) ? v.text : nil
>
> @@ -134,13 +127,13 @@ def condormatic_instances_sync_states
>        Rails.logger.info "job name is #{job_name}"
>        Rails.logger.info "job state is #{job_state}"
>  instance = Instance.find(:first, :conditions => {:condor_job_id => job_name})
>  jobs_state[job_name] = condor_to_instance_state(job_state) if job_name
> end
>  if instance
>    instance.state = condor_to_instance_state(job_state)
>    instance.save!
>    Rails.logger.info "Instance state updated to #{condor_to_instance_state(job_state)}"
>  end
> Instance.find(:all).each do |instance|
>  instance.state = jobs_state[instance.condor_job_id] || Instance::STATE_STOPPED
>  instance.save!
>  Rails.logger.info "Instance state updated to #{instance.state}"
> end
>  rescue Exception => ex
>    Rails.logger.error ex.message

Fancy stuff.

ACK