Opened 2 years ago

Last modified 3 days ago

#6425 enhancement new

Shared time implementation across Clock and reactors

Reported by: itamar Owned by:
Priority: normal Milestone:
Component: core Keywords: review
Cc: firxen@… Branch: branches/shared-time-implementation-6425
(diff, github, buildbot, log)
Author: jerith Launchpad Bug:

Description

It would be nice if twisted.internet.task.Clock were somehow the basis of the IReactorTime implementations in reactors, so we didn't have multiple unrelated implementations of this functionality (and ideally so that our time tests could all be applied to both Clock and reactors).

Attachments (1)

shared-time-implementation.patch (10.3 KB) - added by jerith 10 days ago.

Download all attachments as: .zip

Change History (12)

Changed 10 days ago by jerith

comment:1 Changed 10 days ago by jerith

  • Keywords review added

This is a first stab at factoring out a time implementation that can be shared between the reactor and twisted.internet.task.Clock.

I left a big XXX comment in the new Clock.calls property because it almost certainly needs deprecation and modifying its value no longer affects the object it belongs to. I'm not sure what to do with it, and I'd appreciate any suggestions.

Other than that, the patch is a pretty straightforward factoring out of the DelayedCall-wrangling code from ReactorBase into a class that both ReactorBase and Clock can use. I haven't touched any tests, because this shouldn't change any public behaviour or APIs. (Clock.calls is undocumented, but it isn't underscore-prefixed and it's used in a bunch of tests, so I'm not sure what its status is.)

It's probably worth running benchmarks or something on this to make sure I haven't accidentally hurt performance, but I don't know how to do that. I also haven't added any topfiles yet because it's already a couple of hours past my bedtime. I'll do that tomorrow.

comment:2 Changed 10 days ago by jerith

  • Cc firxen@… added

comment:3 Changed 10 days ago by jerith

  • Author set to jerith
  • Branch set to branches/shared-time-implementation-6425

(In [45542]) Branching to shared-time-implementation-6425.

comment:4 Changed 10 days ago by jerith

(In [45551]) Apply shared-time-implementation.patch from jerith.

Refs: #6425

comment:5 Changed 10 days ago by jerith

I forgot to reference this ticket in [45552]: Add docstrings, fix up twistedchecker violations.

comment:6 Changed 10 days ago by jerith

(In [45553]) Add topfile.

Refs: #6425

comment:8 follow-up: Changed 7 days ago by adiroiban

Hi,

Thanks for working on this.

Here is a quick review.


Why do we have different code in the while self._pendingDelayedCalls: EXECUTE_CALLS loop ?

I was expecting that we can also share that part.


Maybe this code can be extracted into a method on the new PendingDelayCalls.

tple = DelayedCall(self.seconds() + _seconds, _f, args, kw,
	self._pendingDelayedCalls.cancelCallLater,
	self._pendingDelayedCalls.moveCallLaterSooner,
	seconds=self.seconds)
self._pendingDelayedCalls.add(dc)

Once extracted we will have one less public method as we no longer need to export cancelCallLater moveCallLaterSooner
... but we need to see how to handle the self.seconds() ... so maybe there is no gain.


updateCancellations needs a docstring

For the _PendingDelayedCalls docstring. Do we need to mention that it is 'efficient manager' ? Do we have other opttions? Maybe just leave it as 'manager' :) Efficient for that, memory usage or cpu usage or both?


For the XXX comment please create a ticket and add a commment complying with the coding standard:
http://twistedmatrix.com/documents/current/core/development/policy/coding-standard.html#comments

Clock.call is public :( so we might need 2 tickets. 1 to replace all its usage in Twisted own code and then another one to deprecate it.

Since we are on the task.Clock refactoring, maybe we should also document the rightNow instance member.


I think the new code (Even if it is just a move of the old code) needs a few more tests... to have at lease 100% coverage - https://buildbot.twistedmatrix.com/builds/twisted-coverage.py/twisted-fedora21py2.7-coverage.py-r1d5d2c14dedeaf97191b1a2391f3ed79d7ae3ed3/twisted_internet_base.html


This is a quick review as right now I am not in the capabilities of doing a full review... but hope that the comments will help.

I am leaving this in the reivew queue so that other can add more comments.

Thanks for working on this.

comment:9 in reply to: ↑ 8 Changed 7 days ago by jerith

Thanks for the review!

Replying to adiroiban:

Why do we have different code in the while self._pendingDelayedCalls: EXECUTE_CALLS loop ?

I was expecting that we can also share that part.

The reactor catches and logs exceptions, while Clock lets them propagate. Unless we want to change that behaviour (which would break compat), it's hard to share. :-/


Maybe this code can be extracted into a method on the new PendingDelayCalls.

tple = DelayedCall(self.seconds() + _seconds, _f, args, kw,
	self._pendingDelayedCalls.cancelCallLater,
	self._pendingDelayedCalls.moveCallLaterSooner,
	seconds=self.seconds)
self._pendingDelayedCalls.add(dc)

Once extracted we will have one less public method as we no longer need to export cancelCallLater moveCallLaterSooner
... but we need to see how to handle the self.seconds() ... so maybe there is no gain.

I thought about adding a method on _PendingDelatedCalls that just handles the cancel and reset callbacks, but at the time I wasn't sure if those were exposed anywhere. I now know that they aren't, so I'll take another look at doing that. Thanks!


updateCancellations needs a docstring

So it does. I'm not sure how I missed that.


For the _PendingDelayedCalls docstring. Do we need to mention that it is 'efficient manager' ? Do we have other opttions? Maybe just leave it as 'manager' :) Efficient for that, memory usage or cpu usage or both?

It's 'efficient' in CPU usage, but that probably doesn't need to be in the docstring.


For the XXX comment please create a ticket and add a commment complying with the coding standard:
http://twistedmatrix.com/documents/current/core/development/policy/coding-standard.html#comments

I'm intending to remove the XXX in this branch once we know what needs to happen to the attribute.


Clock.call is public :( so we might need 2 tickets. 1 to replace all its usage in Twisted own code and then another one to deprecate it.

Keeping the old mutable behaviour is really hard with the new implementation, so deprecating that likely mean delaying this work until we can remove it. If we don't keep the old mutable behaviour (which isn't used anywhere in Twisted itself, but may be used on other people's code), it would probably be best to explicitly prohibit modification of the list you get. If we keep the attribute, make it read-only, and document it (which is my preferred approach), I don't think we need to deprecate anything. Maybe we could make it read-only and deprecate it in favour of getDelayedCalls which is already part of the IReactorTime interface, though.


Since we are on the task.Clock refactoring, maybe we should also document the rightNow instance member.

Seems reasonable.


I think the new code (Even if it is just a move of the old code) needs a few more tests... to have at lease 100% coverage - https://buildbot.twistedmatrix.com/builds/twisted-coverage.py/twisted-fedora21py2.7-coverage.py-r1d5d2c14dedeaf97191b1a2391f3ed79d7ae3ed3/twisted_internet_base.html

Good point, I'll do that. Thanks!


This is a quick review as right now I am not in the capabilities of doing a full review... but hope that the comments will help.

I am leaving this in the reivew queue so that other can add more comments.

Note to reviewers: I'm leaving this in the review queue while I address the issues mentioned above. I'm particularly interested in feedback about handling Clock.calls.

comment:10 Changed 6 days ago by adiroiban

I am using Clock.calls on my code but I think that I can migrate to getDelayedCalls.

Maybe we can have a new class...task.Scheduler which comes without calls and deprecate the whole task.Clock

I was always confused by the name Clock... it is not only a clock but can also handle the delayed calls.


I have a "standard" test case in which all tests make use of a single task.Clock. The tearDown of the test case checks that no delayed calls are left behind without being handled or explicitly cancelled by the test.
If there are errors, the tearDown clears the clock so that other tests will not continue to fail.

comment:11 Changed 3 days ago by jerith

(In [45575]) Refactor DelayedCall creation and update some docstrings.

Refs: #6425

Note: See TracTickets for help on using tickets.