Skip to content

Commit

Permalink
Cache relationship in Hash rather than in Array.
Browse files Browse the repository at this point in the history
Also cacle only ids, not entire instances.
  • Loading branch information
marshall-lee committed Mar 9, 2019
1 parent 19b6699 commit a952a4b
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 36 deletions.
8 changes: 4 additions & 4 deletions lib/axlsx/package.rb
Original file line number Diff line number Diff line change
Expand Up @@ -100,13 +100,13 @@ def workbook=(workbook) DataTypeValidator.validate :Package_workbook, Workbook,
# File.open('example_streamed.xlsx', 'w') { |f| f.write(s.read) }
def serialize(output, confirm_valid=false)
return false unless !confirm_valid || self.validate.empty?
Relationship.initialize_cached_instances
Relationship.initialize_ids_cache
Zip::OutputStream.open(output) do |zip|
write_parts(zip)
end
true
ensure
Relationship.clear_cached_instances
Relationship.clear_ids_cache
end


Expand All @@ -115,13 +115,13 @@ def serialize(output, confirm_valid=false)
# @return [StringIO|Boolean] False if confirm_valid and validation errors exist. rewound string IO if not.
def to_stream(confirm_valid=false)
return false unless !confirm_valid || self.validate.empty?
Relationship.initialize_cached_instances
Relationship.initialize_ids_cache
zip = write_parts(Zip::OutputStream.new(StringIO.new, true))
stream = zip.close_buffer
stream.rewind
stream
ensure
Relationship.clear_cached_instances
Relationship.clear_ids_cache
end

# Encrypt the package into a CFB using the password provided
Expand Down
47 changes: 20 additions & 27 deletions lib/axlsx/rels/relationship.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,40 +5,40 @@ module Axlsx
class Relationship

class << self
# Keeps track of all instances of this class.
# Keeps track of relationship ids in use.
# @return [Array]
def instances
Thread.current[:axlsx_relationship_cached_instances] ||= []
def ids_cache
Thread.current[:axlsx_relationship_ids_cache] ||= {}
end

# Initialize cached instances.
# Initialize cached idids.
#
# This should be called before serializing a package (see {Package#serialize} and
# {Package#to_stream}) to make sure that serialization is idempotent (i.e.
# Relationship instances are generated with the same IDs everytime the package
# is serialized).
def initialize_cached_instances
Thread.current[:axlsx_relationship_cached_instances] = []
def initialize_ids_cache
Thread.current[:axlsx_relationship_ids_cache] = {}
end

# Clear cached instances.
# Clear cached ids.
#
# This should be called after serializing a package (see {Package#serialize} and
# {Package#to_stream}) to free the memory allocated for cache.
#
# Also, calling this avoids memory leaks (cached instances lingering around
# Also, calling this avoids memory leaks (cached ids lingering around
# forever).
def clear_cached_instances
Thread.current[:axlsx_relationship_cached_instances] = nil
def clear_ids_cache
Thread.current[:axlsx_relationship_ids_cache] = nil
end

# Generate and return a unique id (eg. `rId123`) Used for setting {#Id}.
#
# The generated id depends on the number of cached instances, so using
# {clear_cached_instances} will automatically reset the generated ids, too.
# The generated id depends on the number of previously cached ids, so using
# {clear_ids_cache} will automatically reset the generated ids, too.
# @return [String]
def next_free_id
"rId#{instances.size + 1}"
"rId#{ids_cache.size + 1}"
end
end

Expand Down Expand Up @@ -88,12 +88,7 @@ def initialize(source_obj, type, target, options={})
self.Target=target
self.Type=type
self.TargetMode = options[:target_mode] if options[:target_mode]
@Id = if (existing = self.class.instances.find{ |i| should_use_same_id_as?(i) })
existing.Id
else
self.class.next_free_id
end
self.class.instances << self
@Id = (self.class.ids_cache[ids_cache_key] ||= self.class.next_free_id)
end

# @see Target
Expand All @@ -114,7 +109,7 @@ def to_xml_string(str = '')
str << '/>'
end

# Whether this relationship should use the same id as `other`.
# A key that determines whether this relationship should use already generated id.
#
# Instances designating the same relationship need to use the same id. We can not simply
# compare the {#Target} attribute, though: `foo/bar.xml`, `../foo/bar.xml`,
Expand All @@ -124,13 +119,11 @@ def to_xml_string(str = '')
# then {#Target} will be an absolute URL and thus can safely be compared).
#
# @todo Implement comparison of {#Target} based on normalized path names.
# @param other [Relationship]
def should_use_same_id_as?(other)
result = self.source_obj == other.source_obj && self.Type == other.Type && self.TargetMode == other.TargetMode
if self.TargetMode == :External
result &&= self.Target == other.Target
end
result
# @return [Array]
def ids_cache_key
key = [source_obj, self.Type, self.TargetMode]
key << self.Target if self.TargetMode == :External
key
end

end
Expand Down
6 changes: 3 additions & 3 deletions test/rels/tc_relationship.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ def test_instances_with_same_attributes_share_id
assert_equal instance.Id, Axlsx::Relationship.new(source_obj, Axlsx::WORKSHEET_R, 'target').Id
end

def test_instances_cache_is_thread_safe
def test_ids_cache_is_thread_safe
cache1, cache2 = nil
t1 = Thread.new { cache1 = Axlsx::Relationship.instances }
t2 = Thread.new { cache2 = Axlsx::Relationship.instances }
t1 = Thread.new { cache1 = Axlsx::Relationship.ids_cache }
t2 = Thread.new { cache2 = Axlsx::Relationship.ids_cache }
[t1, t2].each(&:join)
assert_not_same(cache1, cache2)
end
Expand Down
4 changes: 2 additions & 2 deletions test/tc_package.rb
Original file line number Diff line number Diff line change
Expand Up @@ -236,8 +236,8 @@ def test_to_stream
# this is just a roundabout guess for a package as it is build now
# in testing.
assert(stream.size > 80000)
# Cached instances should be cleared
assert(Axlsx::Relationship.instances.empty?)
# Cached ids should be cleared
assert(Axlsx::Relationship.ids_cache.empty?)
end

def test_encrypt
Expand Down

0 comments on commit a952a4b

Please sign in to comment.