On Mon, 2009-01-26 at 08:48 -0600, Luke Kanies wrote:
> On Jan 26, 2009, at 3:32 AM, Brice Figureau wrote:
>
> >
> > On Sun, 2009-01-25 at 21:56 -0600, Luke Kanies wrote:
> >> On Jan 25, 2009, at 3:54 PM, Brice Figureau wrote:
> >>
> >>>
> >>> The rationale behind this patch is that it takes a lots of time
> >>> to let rails unserialize the ParamValue and ResourceTag object
> >>> on each compilation, just to throw them away the second after.
> >>> The idea is to fetch directly (and batched host per host) the
> >>> parameters and tags from the database and then returns them as
> >>> hash.
> >>> This allows the no-modification case to takes at least 2 times
> >>> less than before.
> >>>
> >>> Signed-off-by: Brice Figureau <brice-pup...@daysofwonder.com>
> >>> ---
> >>> lib/puppet/parser/resource.rb | 10 ++++----
> >>> lib/puppet/parser/resource/param.rb | 20 ++++++++--------
> >>> lib/puppet/rails/host.rb | 44 ++++++++++++++++++++++++
> >>> ++
> >>> +++++----
> >>> lib/puppet/rails/resource.rb | 35 +++++++++++++++++++++
> >>> +-----
> >>> 4 files changed, 82 insertions(+), 27 deletions(-)
> >>>
> >>> diff --git a/lib/puppet/parser/resource.rb b/lib/puppet/parser/
> >>> resource.rb
> >>> index 747338b..44979e4 100644
> >>> --- a/lib/puppet/parser/resource.rb
> >>> +++ b/lib/puppet/parser/resource.rb
> >>> @@ -181,11 +181,11 @@ class Puppet::Parser::Resource
> >>> hash
> >>> end
> >>>
> >>> -
> >>> db_resource
> >>> .ar_hash_merge
> >>> (db_resource.get_params_hash(db_resource.param_values),
> >>> updated_params,
> >>> + db_resource.ar_hash_merge(db_resource.get_params_hash(),
> >>> updated_params,
> >>> :create => Proc.new { |name,
> >>> parameter|
> >>> parameter.to_rails(db_resource)
> >>> }, :delete => Proc.new { |values|
> >>> - values.each { |value|
> >>> db_resource.param_values.delete(value) }
> >>> + values.each { |value|
> >>> Puppet::Rails::ParamValue.delete(value['id']) }
> >>> }, :modify => Proc.new { |db, mem|
> >>> mem.modify_rails_values(db)
> >>> })
> >>> @@ -194,13 +194,13 @@ class Puppet::Parser::Resource
> >>> hash[tag] = tag
> >>> hash
> >>> }
> >>> -
> >>> +
> >>> db_resource.ar_hash_merge(db_resource.get_tag_hash(),
> >>> updated_tags,
> >>> :create => Proc.new { |name, tag|
> >>>
> >>> db_resource.add_resource_tag(name)
> >>> }, :delete => Proc.new { |tag|
> >>> -
> >>> db_resource.resource_tags.delete(tag)
> >>> +
> >>> Puppet::Rails::ResourceTag.delete(tag['id'])
> >>> }, :modify => Proc.new { |db, mem|
> >>> # nothing here
> >>> })
> >>> @@ -437,7 +437,7 @@ class Puppet::Parser::Resource
> >>> # 'type' isn't a valid column name, so we have to use
> >>> another name.
> >>> to = (param == :type) ? :restype : param
> >>> if value = self.send(param)
> >>> - hash[to] = value
> >>> + hash[to] = value.is_a?(Symbol) ? value.to_s : value
> >>
> >> We should probably be managing tags on the input side - we should use
> >> the Tagging module to make sure we either always have symbols or
> >> never
> >> do. Would that be out of scope for this?
> >
> > I had to add this because of the generated Class[main], this is not
> > really related to tags, but in fact the Class[main] has the
> > title :main
> > (ie Symbol) and not "main" (String).
> >
> > This meant that the main class was always rewritten to the database
> > because Symbols are internal YAML dumped by rails (ie to "--- :main")
> > and of course this didn't match with "Class[main]" which is what it
> > was
> > compared.
> >
> > I didn't bother looking where this was done, but maybe that's where it
> > should be done (ie using a String instead of a Symbol).
>
> I've been going through the system and making sure everyone uses the
> Tagging module, although I think there are still a few stragglers out
> there who don't.
>
> You *should* be able to just modify that module to convert all tags to
> strings; I've looked quickly through the code, and it looks like none
> of the parsing code still has its own 'tag' method.
Yes, I think.
But the original issue remains, this is not tags but resource
parameters. In particular this is the title of the class "main", or
maybe I'm misunderstanding what you're saying.
> [snip]
> >> Otherwise, it's a question of functionality -- this is mostly a
> >> process of picking uglier but faster code, so behaviour is what
> >> matters most.
> >
> > Yes, that's why I said earlier I wasn't sure you'd accept the patch.
> > I'll work on this only if you think there is an interest and you're
> > willing to merge it. I understand it is ugly (but not as much as was
> > the
> > first incarnation).
> >
> > I also need to do more real work-load tests to see how much the
> > benefit
> > is.
>
>
> If it works, I'll accept the patch; performance optimization is a
> process of moving from the (hopefully) most maintainable code to the
> (hopefully) most performant code, which inevitably means it is less
> maintainable at the end.
OK, then I'll keep working on it this week. I'll take into account your
comments and see if it survives my (small) production load.
Thanks
--
Brice Figureau
My Blog: http://www.masterzen.fr/
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to puppet-dev@googlegroups.com
To unsubscribe from this group, send email to
puppet-dev+unsubscr...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en
-~----------~----~----~----~------~----~------~--~---
