(PUP-3436) Optimize CPU intensitive Compiler methods #3155
Conversation
| @@ -1,7 +1,7 @@ | |||
| require 'puppet/util/tag_set' | |||
|
|
|||
| module Puppet::Util::Tagging | |||
| ValidTagRegex = /^\w[-\w:.]*$/ | |||
| ValidTagRegex = /^[0-9A-Za-z_][0-9A-Za-z_:.-]*$/ | |||
There was a problem hiding this comment.
Why was this change made? Is it faster? The two regexps looks equivalent to me...
There was a problem hiding this comment.
It's significantly faster.
There was a problem hiding this comment.
Wow, that is surprising! I think \w is used in the parser too. Will see if this optimization can be applies there as well. Thanks.
|
Did you measure for Ruby 1.9 as well? |
|
No, not possible since the profiler relies on features introduced in 2.1 |
|
With 1.9 going EOL in Feb 2015, targeting 2.1 makes sense to me. |
|
CLA signed by all contributors. |
| @in_to[ e.target][e.source] ||= []; @in_to[ e.target][e.source] |= [e] | ||
| @out_from[e.source][e.target] ||= []; @out_from[e.source][e.target] |= [e] | ||
| # Avoid multiple lookups here. This code is performance critical | ||
| (@in_to[e.target][e.source] ||= []) << e |
There was a problem hiding this comment.
These aren't equivalent, the first using |= will prevent duplicate instances of 'e' from appearing in the array.
There was a problem hiding this comment.
ok, so three alternatives to maintain uniqueness:
- Go back to using the |= operator and force an unnecessary creation of an extra array instance (well, using a variable it would be once instead of twice).
- Use a Set instead of an Array.
- Test using array.include? before adding.
My preference is 3. Thoughts?
There was a problem hiding this comment.
Is it the second lookup of @foo[e.target][e.source] that is expensive? The creation of an [e] array or the additional calls to e.target and e.source that is expensive?
Trying 3. sounds reasonable, but I don't know if an include? is faster, would be interesting to see.
How expensive is something like:
(@in_to[e.target][e.source] ||= []) |= [e]
compared to
into = @in_to[e.target][e.source] ||= []
into << e if !into.include?(e)
There was a problem hiding this comment.
I'll measure it. But in my mind the former is more expensive since it must do what the latter does anyway. In addition it must:
- create an array
- add an element to the array
- iterate over the array using an index
thallgren
force-pushed
the
compiler-optimizations
branch
2 times, most recently
from
9ad9224
to
44f29d6
Compare
|
Added a new optimization to avoid a lot of unnecessary execution when creating new resources as copies of existing ones. With that in place, the three aforementioned tests execute almost twice as fast as they do without this PR. |
thallgren
force-pushed
the
compiler-optimizations
branch
from
b280915
to
95f15c4
Compare
|
@thallgren bookkeeping: can you file a ticket and reference it in the commits? |
thallgren
force-pushed
the
compiler-optimizations
branch
from
95f15c4
to
95e07cf
Compare
|
@kylog commits updates as requested. |
thallgren
changed the title
|
@thallgren can you update the PR, it has gone stale |
thallgren
force-pushed
the
compiler-optimizations
branch
from
95e07cf
to
5acbd39
Compare
|
PR updated. All builds pass except one that fails when running ruby 1.8.7. I have verified that the failure is unrelated to this PR (it fails on master too). The failing test was introduced 8 days ago in commit 49c5dab and the error is: unsatisfied expectations:
- expected exactly once, not yet invoked: Puppet.warning('pkg warning: [\'Certificate '/var/pkg/ssl/871b4ed0ade09926e6adf95f86bf17535f987684' for publisher 'solarisstudio', needed to access 'https://pkg.oracle.com/solarisstudio/release/', will expire in '29' days.\']')
The code incorrectly assumes that Array.to_s adds brackets. It doesn't in Ruby < 1.9 |
|
@thallgren that was me actually last night. I will take a look, as I don't want to break compatibility with 1.8 for no good reason. |
|
@thallgren should be resolved now |
This commit contains some performance optimizations in methods that showed up as very time consuming in the profiler when running the rake following rake tests: benchmark:many_modules:run benchmark:defined_types4:run benchmark:defined_types:run CPU consumption with this patch in place is between 70% - 75% of what it used to be. Profiling was performed using stackprof https://github.com/tmm1/stackprof
The Resource::copy_as_resource first created a new Resource instance with all bells and whistles which means extracting title, munching, and tagging. This commit ensures that these fields are more effiently initialized when the source is known to be a Resource.
This commit removes an unnecessary cloning of a tag set in the method Puppet::Parser::Resource#add_scope_tags. It also removes unnecessary processing of the tags since they have undergone this processing already.
thallgren
force-pushed
the
compiler-optimizations
branch
from
7bf685f
to
78b354b
Compare
|
@joshcooper looks good now. Thanks. |
|
sigh - this again needs a rebase - I am doing that locally and will merge this in. |
|
Manually merged at 2d0e0a6 - closing |
This commit contains some performance optimizations in methods
that showed up as very time consuming in the profiler when running
the rake following rake tests:
benchmark:many_modules:run
benchmark:defined_types4:run
benchmark:defined_types:run
CPU consumption with this patch in place is between 70% - 75% of
what it used to be.
Profiling was performed using stackprof
https://github.com/tmm1/stackprof