Permalink
Browse files

Don't re-require 'rexml/document'

  • Loading branch information...
lifo committed Nov 26, 2008
1 parent 05a938c commit 17940a82e8cb0ed278e1552b943dd033763978a1
Showing with 1 addition and 1 deletion.
  1. +1 −1 activesupport/lib/active_support/xml_mini.rb
@@ -17,7 +17,7 @@ module XmlMini
# string::
# XML Document string to parse
def parse(string)
- require 'rexml/document'
+ require 'rexml/document' unless defined?(REXML::Document)
doc = REXML::Document.new(string)
merge_element!({}, doc.root)
end

12 comments on commit 17940a8

@qerub

This comment has been minimized.

Show comment Hide comment
@qerub

qerub Nov 26, 2008

Contributor

In what case will this make a difference?

require only loads files once if $LOAD_PATH is set up properly.

Contributor

qerub replied Nov 26, 2008

In what case will this make a difference?

require only loads files once if $LOAD_PATH is set up properly.

@lifo

This comment has been minimized.

Show comment Hide comment
@lifo

lifo Dec 1, 2008

Member

defined? is a zillion times faster than require

Member

lifo replied Dec 1, 2008

defined? is a zillion times faster than require

@sr

This comment has been minimized.

Show comment Hide comment
@sr

sr Dec 1, 2008

here is a bench that proves it: http://gist.github.com/24726

sr replied Dec 1, 2008

here is a bench that proves it: http://gist.github.com/24726

@qerub

This comment has been minimized.

Show comment Hide comment
@qerub

qerub Dec 1, 2008

Contributor

Thanks for the explanation!

Has anybody analysed and documented why require is so slow?

Contributor

qerub replied Dec 1, 2008

Thanks for the explanation!

Has anybody analysed and documented why require is so slow?

@henrik

This comment has been minimized.

Show comment Hide comment
@henrik

henrik Dec 1, 2008

Contributor

This is definitely double-take code. If it’s a one-off, how about a comment explaining why? If it’s not a one-off, it should probably be wrapped in a method (with an explanation there).

Contributor

henrik replied Dec 1, 2008

This is definitely double-take code. If it’s a one-off, how about a comment explaining why? If it’s not a one-off, it should probably be wrapped in a method (with an explanation there).

@qerub

This comment has been minimized.

Show comment Hide comment
@qerub

qerub Dec 2, 2008

Contributor

Another instance of the same pattern:
http://github.com/rails/rails/commit/27dbc27c4174974a318145d2dc6512457ea37241

Contributor

qerub replied Dec 2, 2008

Another instance of the same pattern:
http://github.com/rails/rails/commit/27dbc27c4174974a318145d2dc6512457ea37241

@qerub

This comment has been minimized.

Show comment Hide comment
@qerub

qerub Dec 2, 2008

Contributor

Here’s a list of all instances in rails/HEAD:
http://pastie.textmate.org/private/nz0pnkfusyesyguofgunea

Contributor

qerub replied Dec 2, 2008

Here’s a list of all instances in rails/HEAD:
http://pastie.textmate.org/private/nz0pnkfusyesyguofgunea

@defunkt

This comment has been minimized.

Show comment Hide comment
@defunkt

defunkt Dec 2, 2008

Contributor

Run $LOAD_PATH.size in both a Rails console and normal irb then compare the results.

Contributor

defunkt replied Dec 2, 2008

Run $LOAD_PATH.size in both a Rails console and normal irb then compare the results.

@NZKoz

This comment has been minimized.

Show comment Hide comment
@NZKoz

NZKoz Dec 2, 2008

Member

In addition to the size of the load path, there’s the gems that you have, and the fact that rubygems monkeypatches require so it’ll take a while longer.

The reality is we shouldn’t need to put this here, requiring in methods is nasty. Hopefully once the autoload changes settle down we can make this a little nicer.

As for DRYing it up, we can’t wrap it in a method anyway as defined? has special behaviour you can’t replicate in your own code

Member

NZKoz replied Dec 2, 2008

In addition to the size of the load path, there’s the gems that you have, and the fact that rubygems monkeypatches require so it’ll take a while longer.

The reality is we shouldn’t need to put this here, requiring in methods is nasty. Hopefully once the autoload changes settle down we can make this a little nicer.

As for DRYing it up, we can’t wrap it in a method anyway as defined? has special behaviour you can’t replicate in your own code

@datra

This comment has been minimized.

Show comment Hide comment
@datra

datra Dec 2, 2008

So this is not possible?

def require_unless_defined(library, class)
  require library unless defined?(class)
end

datra replied Dec 2, 2008

So this is not possible?

def require_unless_defined(library, class)
  require library unless defined?(class)
end
@NZKoz

This comment has been minimized.

Show comment Hide comment
@NZKoz

NZKoz Dec 2, 2008

Member

No, it’s not.

> > require_unless_defined(“asdf”, REXML::Document)
> > NameError: uninitialized constant REXML
> > from (irb):4

Member

NZKoz replied Dec 2, 2008

No, it’s not.

> > require_unless_defined(“asdf”, REXML::Document)
> > NameError: uninitialized constant REXML
> > from (irb):4

@datra

This comment has been minimized.

Show comment Hide comment
@datra

datra Dec 2, 2008

Okay, thanks. I was just curious. :)

datra replied Dec 2, 2008

Okay, thanks. I was just curious. :)

Please sign in to comment.