Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Cleanup usage of Actor class methods by using the method proxies #201

Merged
merged 8 commits into from Mar 28, 2013

Conversation

Projects
None yet
4 participants
Owner

halorgium commented Mar 27, 2013

I really felt like the Celluloid::Actor methods are being overused and are somewhat unnecessary.

This unifies a lot of the calls onto the method proxies inside the ActorProxy.

It also introduces a ActorProxy#sync method for doing explicit sync calls when there is a possibility of ambiguity.

Coverage remained the same when pulling 5e9a064 on halorgium:call-cleanups into a165238 on celluloid:master.

View Details

@benlangfeld benlangfeld commented on an outdated diff Mar 27, 2013

lib/celluloid/proxies/actor_proxy.rb
@@ -64,10 +65,19 @@ def to_s
Actor.call @mailbox, :to_s
end
+ # Obtain an sync proxy or explicitly invoke a named async method
@benlangfeld

benlangfeld Mar 27, 2013

Member

s/async/sync

Owner

tarcieri commented Mar 27, 2013

Kinda curious what effect this has on performance since you're now going through three objects instead of two. Have you tried the benchmarks?

Owner

halorgium commented Mar 27, 2013

@tarcieri seems that it gets faster? https://gist.github.com/halorgium/3439d6e173bf8297b7cb
This is on 2.0.0-p0.

Owner

halorgium commented Mar 27, 2013

There seems to be high variance on these benchmarks.

Coverage remained the same when pulling 3660a1c on halorgium:call-cleanups into a165238 on celluloid:master.

View Details

Coverage remained the same when pulling 45fac92 on halorgium:call-cleanups into a165238 on celluloid:master.

View Details

Owner

halorgium commented Mar 28, 2013

@tarcieri I have updated this to make ActorProxy a subclass of SyncProxy.
Once we have the delegate.rb stuff figured out, it will be simple enough IMO to just replace AbstractProxy and everything will work.

Owner

halorgium commented Mar 28, 2013

The main problem is the __class__ which is now impossible to implement with BasicObject.
2 specs fail due to this, is this something we can test through behaviour of the proxy_class instead of testing the class name itself.

Owner

halorgium commented Mar 28, 2013

OK, that is fixed.
All the punchblock specs pass with this branch.

Coverage remained the same when pulling 4209242 on halorgium:call-cleanups into a165238 on celluloid:master.

View Details

@tarcieri tarcieri added a commit that referenced this pull request Mar 28, 2013

@tarcieri tarcieri Merge pull request #201 from halorgium/call-cleanups
Cleanup usage of Actor class methods by using the method proxies
7c9e9bc

@tarcieri tarcieri merged commit 7c9e9bc into celluloid:master Mar 28, 2013

1 check passed

default The Travis build passed
Details
Owner

tarcieri commented Mar 28, 2013

Cool, I'll call this forward progress ;)

Owner

halorgium commented Mar 28, 2013

;)

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