Grouping thread locals in the ActiveRecord scopes #10135

Merged
merged 1 commit into from Apr 8, 2013

Conversation

Projects
None yet
2 participants
@wangjohn
Contributor

wangjohn commented Apr 8, 2013

I'm grouping some thread locals under a registry object so that we can try to get the thread locals under control. I've brought the current_scope and ignore_default_scope locals together into a single object.

Accesses to these thread locals are now governed through the ScopeRegistry object.

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Apr 8, 2013

Owner

I think the main reason that @tenderlove want to do this is to lower the number of thread current variables and this current implementation is not changing anything.

I may be wrong but I think we have to do something like this:

class ScopeRegistry
  def self.current
    Thread.current["scope_registry"] ||= new
  end

  def initialize
    @current_scopes = {}
    @ignore_default_scopes = {}
  end

  def current_scope_for(class_name)
    @current_scopes[class_name]
  end

  def set_current_scope_for(class_name, value)
    @current_scopes[class_name] = value
  end

  ...
Owner

rafaelfranca commented Apr 8, 2013

I think the main reason that @tenderlove want to do this is to lower the number of thread current variables and this current implementation is not changing anything.

I may be wrong but I think we have to do something like this:

class ScopeRegistry
  def self.current
    Thread.current["scope_registry"] ||= new
  end

  def initialize
    @current_scopes = {}
    @ignore_default_scopes = {}
  end

  def current_scope_for(class_name)
    @current_scopes[class_name]
  end

  def set_current_scope_for(class_name, value)
    @current_scopes[class_name] = value
  end

  ...
@wangjohn

This comment has been minimized.

Show comment Hide comment
@wangjohn

wangjohn Apr 8, 2013

Contributor

@rafaelfranca Ah! Lowering the number of thread locals makes sense. I'll change the implementation to something like what you gave.

Contributor

wangjohn commented Apr 8, 2013

@rafaelfranca Ah! Lowering the number of thread locals makes sense. I'll change the implementation to something like what you gave.

@wangjohn

This comment has been minimized.

Show comment Hide comment
@wangjohn

wangjohn Apr 8, 2013

Contributor

@rafaelfranca I've rewritten the PR so that there is only a single thread local for all the scopes. There's now a single registry object which keeps track of everything.

Contributor

wangjohn commented Apr 8, 2013

@rafaelfranca I've rewritten the PR so that there is only a single thread local for all the scopes. There's now a single registry object which keeps track of everything.

+ # registry.value_for(:current_scope, "Board")
+ #
+ # You will obtain whatever was defined in +some_new_scope+.
+ class ScopeRegistry

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Apr 8, 2013

Owner

I don't think this class should be part of the public API, so I'd add a # :nodoc: here.

@rafaelfranca

rafaelfranca Apr 8, 2013

Owner

I don't think this class should be part of the public API, so I'd add a # :nodoc: here.

@rafaelfranca

View changes

activerecord/lib/active_record/scoping.rb
+ attr_accessor :registry
+
+ def initialize
+ @registry = {}

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Apr 8, 2013

Owner
@registry = Hash.new { |hash, key| hash[key] = {} }

So we don't need to loop on the VALID_SCOPE_TYPES

@rafaelfranca

rafaelfranca Apr 8, 2013

Owner
@registry = Hash.new { |hash, key| hash[key] = {} }

So we don't need to loop on the VALID_SCOPE_TYPES

Grouping thread locals in the ActiveRecord scopes so that the
current_scope and ignore_default_scope locals are brought together under
a registry object.
@wangjohn

This comment has been minimized.

Show comment Hide comment
@wangjohn

wangjohn Apr 8, 2013

Contributor

@rafaelfranca Thanks for the comments, I've updated the PR.

Contributor

wangjohn commented Apr 8, 2013

@rafaelfranca Thanks for the comments, I've updated the PR.

rafaelfranca added a commit that referenced this pull request Apr 8, 2013

Merge pull request #10135 from wangjohn/grouping_thread_locals
Grouping thread locals in the ActiveRecord scopes

@rafaelfranca rafaelfranca merged commit e2e9eed into rails:master Apr 8, 2013

@wangjohn wangjohn deleted the wangjohn:grouping_thread_locals branch Apr 8, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment