Skip to content

Commit

Permalink
Avoid adding constants to Enumerable
Browse files Browse the repository at this point in the history
Ref: aws/aws-sdk-ruby#2670

Some gems like aws-sdk-core use `Object#extend(Enumerable)`.
It's not a very good pattern, but it's somehwat handled ok by Ruby.

However if Enumerable has constants, then any time the module is
extended, the global constant cache is flushed and this has a very
negative impact on performance for the virtual machine, and even
worse for JITs.
  • Loading branch information
byroot committed Mar 4, 2022
1 parent b66606a commit 2cec14a
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 9 deletions.
35 changes: 26 additions & 9 deletions activesupport/lib/active_support/core_ext/enumerable.rb
@@ -1,13 +1,30 @@
# frozen_string_literal: true

module Enumerable
INDEX_WITH_DEFAULT = Object.new
private_constant :INDEX_WITH_DEFAULT
module ActiveSupport
module EnumerableCoreExt # :nodoc:
module Constants
private
def const_missing(name)
if name == :SoleItemExpectedError
::ActiveSupport::EnumerableCoreExt::SoleItemExpectedError
else
super
end
end
end
end
end

module Enumerable
# Error generated by +sole+ when called on an enumerable that doesn't have
# exactly one item.
class SoleItemExpectedError < StandardError; end

# HACK: For performance reasons, Enumerable shouldn't have any constants of its own.
# So we move SoleItemExpectedError into ActiveSupport::EnumerableCoreExt.
ActiveSupport::EnumerableCoreExt::SoleItemExpectedError = remove_const(:SoleItemExpectedError)
singleton_class.prepend(ActiveSupport::EnumerableCoreExt::Constants)

# Enumerable#sum was added in Ruby 2.4, but it only works with Numeric elements
# when we omit an identity.

Expand Down Expand Up @@ -106,17 +123,17 @@ def index_by
#
# %i( created_at updated_at ).index_with(Time.now)
# # => { created_at: 2020-03-09 22:31:47, updated_at: 2020-03-09 22:31:47 }
def index_with(default = INDEX_WITH_DEFAULT)
def index_with(default = (no_default = true))
if block_given?
result = {}
each { |elem| result[elem] = yield(elem) }
result
elsif default != INDEX_WITH_DEFAULT
elsif no_default
to_enum(:index_with) { size if respond_to?(:size) }
else
result = {}
each { |elem| result[elem] = default }
result
else
to_enum(:index_with) { size if respond_to?(:size) }
end
end

Expand Down Expand Up @@ -240,8 +257,8 @@ def in_order_of(key, series)
def sole
case count
when 1 then return first # rubocop:disable Style/RedundantReturn
when 0 then raise SoleItemExpectedError, "no item found"
when 2.. then raise SoleItemExpectedError, "multiple items found"
when 0 then raise ActiveSupport::EnumerableCoreExt::SoleItemExpectedError, "no item found"
when 2.. then raise ActiveSupport::EnumerableCoreExt::SoleItemExpectedError, "multiple items found"
end
end
end
Expand Down
9 changes: 9 additions & 0 deletions activesupport/test/core_ext/enumerable_test.rb
Expand Up @@ -360,4 +360,13 @@ def test_sole
assert_raise(expected_raise) { GenericEnumerable.new([1, 2]).sole }
assert_raise(expected_raise) { GenericEnumerable.new([1, nil]).sole }
end

def test_doesnt_bust_constant_cache
skip "Only applies to MRI" unless defined?(RubyVM.stat) && RubyVM.stat(:global_constant_state)

object = Object.new
assert_no_difference -> { RubyVM.stat(:global_constant_state) } do
object.extend(Enumerable)
end
end
end

0 comments on commit 2cec14a

Please sign in to comment.