Perf tweaks #447

Merged
merged 2 commits into from Jun 26, 2013
@@ -1880,6 +1880,11 @@ def method_missing(sym, *a, &b) # :nodoc:
end
end
+ # Prevent ruby hitting spec.method_missing when [[spec]].flatten is called
+ def to_ary # :nodoc:
+ nil
+ end
+
##
# Normalize the list of files so that:
# * All file lists have redundancies removed.
View
@@ -153,7 +153,10 @@ class Gem::Version
##
# A string representation of this Version.
- attr_reader :version
+ def version
+ @version.dup
+ end
+
alias to_s version
##
@@ -183,6 +186,12 @@ def self.create input
end
end
+ @@all = {}
+
+ def self.new version
@evanphx

evanphx Feb 8, 2013

Owner

Using a Version cache has far reaching implications that I'm not sure we're ready for. It's been discussed before.

@jonleighton

jonleighton Feb 8, 2013

Contributor

Would you accept a global cache for the result of Version#segments as a first step? That's where the real hotspot is.

Also, what sort of implications are you referring to? Out of interest. (Happy to read the discussion if you can point me to it.)

@jonleighton

jonleighton Feb 8, 2013

Contributor

I guess both have thread safety issues. I'm not clear about what the thread safety requirements are for rubygems. Can you fill me in?

@evanphx

evanphx Feb 8, 2013

Owner

There are a couple of considerations:

  1. Versions weren't always immutable. I'm not certain we have finished cleaning things up so they are. If they're not immutable, a global cache is bad news.
  2. For anything that is long lived, this will cause a slowly ballooning memory because there is no way to clean up all the Version objects even if they're not in use.

Both could be dealt with by having an explicit VersionCache object that is used to memoize the Version objects into. It could be that by default, there is no such object and we just create a new object each time. But if one is installed into Specification, it is used. This would allow gem loading to create one and install it for use.

@evanphx

evanphx Feb 9, 2013

Owner

Per, I mean that Version, the class, could be told "use this VersionCache object", not Specification.

@jonleighton

jonleighton Feb 9, 2013

Contributor

Regarding (1), I am pretty sure it's not an issue. From looking at the code, Version is immutable so long as you don't do crazy things like version.hash << "wtf". Incidentally you can see I changed #version to return a dup of the @version as not doing this did lead to a test failure.

Regarding (2), I agree that this is a problem in theory, although in practise I can't see a situation where any application will use an infinite number of different version objects. However, people do do unexpected things so I'm not averse to addressing this concern. I think the simplest thing to do would be to just make the cache bounded to a certain size. We'd have to do some research to find out what a good default for this would be but that would prevent the memory usage growing unbounded. Regarding your VersionCache proposal, I don't see how this would prevent the memory leak specifically? It seems more to do with how to structure the code?

Anyway, maybe we should just return to these when #435 is done, as that's certainly the lowest hanging fruit right now.

+ @@all[version] ||= super
+ end
+
##
# Constructs a Version from the +version+ string. A version string is a
# series of digits or ASCII letters separated by dots.