Memoize Utils::version_geq calls #346

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
Contributor

cheald commented Feb 8, 2011

In profiling my app, I found that version_geq and version_gt were being called about 1300 times per pageview, and each call to version_gt creates multiple arrays and does various things to them. Under profiling, this was responsible for about 120ms of runtime; after this change, it runs fast enough that the profiler doesn't include it in the output, and page render times are reduced by about ~5%.

twmills commented Jun 17, 2011

Yes please!

tejo commented Jun 17, 2011

+1

+1 please get this guy in there

+1. Awesome work!

jc00ke commented Jun 17, 2011

+1 As long as the specs pass, I'd love to see these improvements pulled.

lamp commented Jun 17, 2011

+1

tadast commented Jun 17, 2011

+1 people love performance! And haml is NOT a splinter in our brain :)

Contributor

cheald commented Jun 17, 2011

@jc00ke - the test suite passes, with the exception of one test that is broken in origin/master anyhow (that is to say, my changes didn't break it).

+1 to this (and thanks to @cheald for the great explanation of the changes!).

JDutil commented Jun 17, 2011

+1

+1 but why the permission change: 100644 -> 100755 not that it should matter

Contributor

cheald commented Jun 18, 2011

The permission change is a mistake - sorry about that!

kitop commented Jun 18, 2011

+1

Since this is a global, I'd recommend moving it outside the method and just doing @@ version_comparison_cache = {} somewhere in the class. Otherwise you're testing the variable everytime this method executes, when it will only be nil once.

You could probably get slightly faster performance by using Hash#key? and Hash#fetch here, eg:

 return @@version_comparison_cache.fetch(k) if @@version_comparison_cache.key?(k)

This assumes of course that when the key is set that the value is non-nil, which I think is the case here.

Contributor

cheald commented Jun 18, 2011

Both great points - I'll test and fix them. Thanks!

zquestz commented Jun 18, 2011

Nice work, would like to see this in master.

andreas commented Jun 18, 2011

+1

yannski commented Jun 18, 2011

+1

tmikoss commented Jun 18, 2011

+1

@Roman2K Roman2K commented on the diff Jun 18, 2011

lib/haml/util.rb
def version_geq(v1, v2)
- version_gt(v1, v2) || !version_gt(v2, v1)
+ k = "#{v1}#{v2}"
+ return @@version_comparison_cache.fetch(k) if @@version_comparison_cache.key?(k)
+ @@version_comparison_cache[k] = ( version_gt(v1, v2) || !version_gt(v2, v1) )
@Roman2K

Roman2K Jun 18, 2011

Contributor

Instead of the return ... if ... key? ... or assignment, you could use the block form of Hash#fetch:

@@version_comparison_cache.fetch "#{v1}#{v2}" do |key|
  @@version_comparison_cache[key] = version_gt(v1, v2) || !version_gt(v2, v1)
end

factore commented Jun 18, 2011

+1

@Roman2K Roman2K commented on the diff Jun 18, 2011

lib/haml/compiler.rb
@@ -443,8 +443,11 @@ END
def compile(node)
parent, @node = @node, node
- block = proc {node.children.each {|c| compile c}}
- send("compile_#{node.type}", &(block unless node.children.empty?))
+ if node.children.empty?
+ send(:"compile_#{node.type}")
+ else
+ send(:"compile_#{node.type}") { node.children.each {|c| compile c} }
+ end
@Roman2K

Roman2K Jun 18, 2011

Contributor

The check node.children.empty? is useless since if node.children is empty, the block (node.children.each { |c| compile c }) will be called and do nothing anyway (each on an empty Array).

One would think it's an optimisation to not pass a block that calls each on a empty Array, but I doubt calling empty? on node.children every time is that much cheaper. So I suggest:

def compile(node)
  parent, @node = @node, node
  send "compile_#{node.type}" do
    node.children.each do |child|
      compile child
    end
  end
end

Also note that the dynamically generated method name is not converted to a Symbol prior to passing it to send, since it's useless too. I'm sure Object#send actually deals with Symbols internally, but if so, it's its job to do the appropriate conversion as necessary, not the caller's.

Xorlev commented Jun 18, 2011

This guy has the right idea. I'd love if this was merged into HAML.

+1 please

Contributor

cheald commented Jun 18, 2011

@Roman2K - the reason I rewrote #compile as I did was to avoid unnecessary block binding, not to avoid invoking #each on the array. I'm not quite clear on the rules of implicit block binding, but I know that explicit bindings are certainly slower than their implicit counterparts. I've got too much CPU flux on my dev machine to get a useful benchmark at the moment, but if you want to benchmark out the two versions, I'd appreciate it!

tallica commented Jun 19, 2011

+1

@ghost

ghost commented Jun 19, 2011

+1 thanks

Please merged it! :)

+1

+1 Yes, please.

+1

+1

ezkl commented Jun 21, 2011

+1

+1

bjensen commented Jun 23, 2011

+1

+1

@thetamind thetamind commented on the diff Jun 23, 2011

lib/haml/util.rb
@@ -264,9 +266,11 @@ module Haml
#
# @param v1 [String] A version string.
# @param v2 [String] Another version string.
- # @return [Boolean]
+ # @return [Boolean]
@thetamind

thetamind Jun 23, 2011

Extra trailing whitespace.

czj commented Jun 23, 2011

+1

ivar commented Jun 23, 2011

it'd be nice to see this go in..

+1

+1

@ghost

ghost commented Jun 24, 2011

What are you waiting for?
Please merge this.

ashchan commented Jun 25, 2011

+1

tuo commented Jun 27, 2011

+1

+1

nsanta commented Jun 27, 2011

+1

paneq commented Jun 28, 2011

Subscribe me!

milj commented Jun 28, 2011

+1

+1

mping commented Jul 3, 2011

+1

KensoDev commented Jul 5, 2011

+1

xanview commented Jul 8, 2011

+1!

+1

ahmeij commented Jul 18, 2011

+1

mkremer commented Jul 18, 2011

+1 :)

Contributor

masterkain commented Jul 18, 2011

I approve, +1

axelarge commented Aug 7, 2011

+1 more

+1 more, yes please

dear haml, why you hates speed?

  • ruby

is anyone else shocked that this still hasn't been merged in?

This really needs to be merged in.

cice commented Oct 2, 2011

+1 at least a discussion, why it's not being merged, would be appreciated

ttilley commented Oct 2, 2011

my guess is that this kind of memoization could add complexity and decrease maintainability. besides, it adds runtime state for something that doesn't necessarily require runtime state. the beauty of ruby allows for code to be run at class definition as a way of defining that class. one could also solve this problem by running code at class level: if within this version range, define this method as doing X, otherwise define it as doing Y. goodbye runtime overhead.

</peanut gallery>

Contributor

cheald commented Oct 2, 2011

@ttilley If you look at how the code's being used, runtime state isn't a concern in this case unless your Ruby version changes mid-execution, in which case you have bigger issues. version_geq may be invokes with varying arguments, so a redefine-once approach wouldn't work.

You could rewrite this with method_missing and dynamic function definition as well, but I'm not sure it'd be any more or less maintainable than my current solution.

In any case, it's a ~30% overall performance gain that the Haml devs have left on the table for upwards of 8 months now. Some fix needs to be made, even if it's not congruent with my approach.

ttilley commented Oct 2, 2011

@cheald I was only referring to @@version_comparison_cache when I said runtime state. Reading through the code, I see that any checks I can find are already done at definition time. Now I'm insanely curious where 1500 calls to version_gt are coming from at runtime... even for development mode. Perhaps a better understanding of where these calls are coming from will push this pull request forward? I'm not a repo collaborator, so my opinion doesn't matter, but regardless of the outcome that would be useful information to document.

Contributor

glebm commented Oct 10, 2011

+1

gravis commented Oct 17, 2011

+1

andruby commented Oct 25, 2011

+1

+1

If not this pull request, at least address this demonstrated issue.

+1 This patch looks great.

@nex3, @norman: Why didn't you respond to this for months?

@cheald: Did you talk to those guys by email or on the mailing list?

Contributor

cheald commented Nov 22, 2011

I've not raised this issue outside of Github. Good idea to raise it on the mailing list.

Owner

norman commented Nov 22, 2011

@SaschaKonietzke I'm not actually a committer to Haml. To me, the patch looks fine, but it's not really my call to make.

Collaborator

nex3 commented Nov 22, 2011

Closed by dc5fca4.

I apologize for taking so long to merge this in.

nex3 closed this Nov 22, 2011

@iHiD iHiD pushed a commit to iHiD/haml that referenced this pull request Jan 22, 2012

@nex3 nex3 Merge branch 'perf-fixes'
Closes gh-346
dc5fca4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment