From: Jano Svitok <jan.svitok@...>
Date: 2008-04-03T02:38:30+09:00
Subject: Re: GC help

Few suggestions, some not really related to the problem:

On Wed, Apr 2, 2008 at 6:52 PM, Joey Marino <joey.da3rd@gmail.com> wrote:
> Ok, I was able to get it all into one class. The problem lies in this class:
>  class Picture
>
>   def initialize(db,rets,rets_class)
>    @db = db
>    @rets = rets
>    @rets_class = rets_class
>    @attempts = 0
>   end
>
>
>   def getPic(key)
>    begin
>      get_object_request = GetObjectRequest.new(@rets_class, "Photo")
>      get_object_request.add_all_objects(key)
>      get_object_response = @rets.session.get_object(get_object_request)
>      content_type_suffixes = { "image/jpeg" => "jpg"}

Make content_type_suffixes a class constant, or member if you need to
append to it.
Now you are constructing and destructing the object on each method call.

>      makePicDir(key)
>      get_object_response.each_object do |object_descriptor|
>        object_key =  object_descriptor.object_key
>        obj_id = object_descriptor.object_id
>        content_type = object_descriptor.content_type
>        description = object_descriptor.description
>        #print "#{object_key} object \##{object_id}"
-        #print ", description: #{description}" if !description.empty?
+        #print ", description: #{description}" unless
description.empty? # a matter of taste/style
>        #puts
>        suffix = content_type_suffixes[content_type]
>        pic = object_descriptor.data_as_string
>        savePic(key,obj_id.to_s,suffix,description,pic)
>      end
>      get_object_response = nil
>   rescue => e
>     puts "Error retrieving pictures for #{key}: " + e
>     if @attempts <= 5
>       @attempts += 1
>       puts "retrying"
>       retry
>     else
>       puts "failed"
>       @attempts = 0
>     end
>   end
>   @attempts = 0
>   end
>

It seems that you could refactor the common code of these two methods
in to a new one.
The benefit would be shorter/more readable code and better
responsibility split, the drawback slower execution.

>   def getThumb(key)
>    begin
>      get_object_request = GetObjectRequest.new(@rets_class, "Thumbnail")
>      get_object_request.add_all_objects(key)
>      get_object_response = @rets.session.get_object(get_object_request)
>      content_type_suffixes = { "image/jpeg" => "jpg"}
>      get_object_response.each_object do |object_descriptor|
>        object_key =  object_descriptor.object_key
>        obj_id = object_descriptor.object_id
>        content_type = object_descriptor.content_type
>        description = object_descriptor.description
>        #print "#{object_key} object \##{object_id}"
>        #print ", description: #{description}" if !description.empty?
>        #puts
>        suffix = content_type_suffixes[content_type]
>        pic = object_descriptor.data_as_string
>        savePic(key,obj_id.to_s,suffix,description,pic,true)
>      end
>      get_object_response = nil
>   rescue => e
>     puts "Error retrieving thumbs for #{key}: " + e
>     if @attempts <= 5
>       @attempts += 1
>       puts "retrying"
>       retry
>     else
>       puts "failed"
>       @attempts = 0
>     end
>   end
>   @attempts = 0
>   end
>
>
>   def makePicDir(key)
>    FileUtils.mkpath("#{$pic_dir}#{key}/thumb")
>   end
>
>
>
>
>   def savePic(key,id,suffix,desc,pic,thumb_bool=false)
>    if thumb_bool
>      file_name = $pic_dir + key + "/thumb/" + id + "." + suffix
>      location = "/" + key + "/thumb/" + id + "." + suffix
>    else
>      file_name = $pic_dir + key + "/" + id + "." + suffix
>      location = "/" + key + "/" + id + "." + suffix
>    end
>    self.savePicFile(file_name,pic)

self is not necessary, the same below

>    size = File.size(file_name)
>    if thumb_bool
>      self.insertThumbDB(key,id,location)
>    else
>      self.insertPicDB(key,id,desc,size,location)
>    end
>   end
>
>
>
>
>   def savePicFile(file_name,pic)
-    f = File.open(file_name, "wb")
+     File.open(file_name, "wb") do |f|
>    f << pic
-    f.close
+     end # automatic close on exceptions
>   end
>
>
>
>
>   def insertPicDB(key,id,desc,size,location)
>    description = @db.database.escape_string(desc)
>    if
>  @db.DBinsert("PICS","pkey,id,description,size,location","#{key},#{id},'#{description}','#{size}','#{location}'")
>    # puts "#{key} #{id} pic added"
>      print ":"
>    end
>   end
>
>
>   def insertThumbDB(key,id,location)
>    if @db.DBupdate("PICS","thumb = '#{location}'"," pkey = #{key} and id = #{id}")
>     # puts "#{key} #{id} thumb added"
>       print "."
>    end
>   end
>
>
>
>   def deletePic(key)
>    self.deletePicDir(key)
>    self.deletePicDB(key)
>   end
>
>
>   def deletePicDir(key)
-    if File.exists?("#{$pic_dir}#{key}")
-      FileUtils.remove_dir("#{$pic_dir}#{key}")
-    end
+   pic_dir = $pic_dir + key # save one allocation
+   FileUtils.remove_dir pic_dir if File.directory? pic_dir
>   end
>
>
>
>   def deletePicDB(key)
>   if @db.DBdelete("PICS","pkey = #{key}")
>     print "-"
>  #    puts "#{key} pics deleted from db"
>   end
>   end
>
>  end #end class

I'd suspect the database code is keeping some cache. This code seems fine.

I see you are using libRETS. Did you try to rule it out by replacing
calls to libRETS (especially to data_as_string) with some stubs
(create a random long string on the fly).
If you are on unix, you can use IO.read("/dev/random", 100000). If you
are on Windows, choose another long enough file to read.

The documentation says that GetData() abandons ownership to the object
it returns. It's possible that SWIG-generated wrapper doesn't handle
this properly.

Jano