minor optimistaion

[casperdcl]

alternative to 2d45eca

let me know if this doesn't work @lrq3000

[codecov-io]

Current coverage is 91.76% (diff: 50.00%)

Merging #288 into master will decrease coverage by 0.01%

@@             master       #288   diff @@
==========================================
  Files             7          7          
  Lines           535        534     -1   
  Methods           0          0          
  Messages          0          0          
  Branches         97         97          
==========================================
- Hits            491        490     -1   
  Misses           43         43          
  Partials          1          1          

Powered by Codecov. Last update a65e347...6b73b5b

[CrazyPython]

Nothing peculiar.

[lrq3000]

Hey Casper,
I see what you tried to do, initially I thought it was OK but in fact it's not, because there are multiple instructions in one statement:

self.start_t = self.last_print_t = self._time()

To avoid racing conditions, we need an atomic operation, so either a lock or a single opcode. So this would work:

self.last_print_t = self._time()
self.start_t = self.last_print_t

But then this is less elegant (and maybe less efficient?) since we use two different assignments (I believe Python has some optimizations when we do multiple assignments at once).

So this should work, but I think that having an additional started boolean flag is as good and less confusing (it's dedicated to this task). This incurs a small (negligible) overhead only on initialization. Also, I think it will be easier to evolve, because racing issues are reported more and more (#285) and we'll have to revamp the core a bit to prevent racing issues.

[casperdcl]

Interesting point. Assignment is a language intrinsic, but a few of my tests shows that

self.start_t = self.last_print_t = self._time()

should be

self.last_print_t = self.start_t = self._time()

in order to ensure self.start_t is the final assignment. The code I used was:

"""
>>> a=Foo('a')
>>> b=Foo('b')
>>> c=Foo('c')
>>> a.A = b.B = 1
setting A 1 in a
setting B 1 in b
"""
class Foo:
  def __init__(self, name):
    self.__dict__["name"] = name
  def __setattr__(self, k, v):
    print "setting", k, v, "in", self.name

let me know if you get different behaviour. It would be nice to avoid a "started" variable until we actually need it in another PR

[lrq3000]

I get the same but we should not rely on this, because the implementation of multiple assignments order might change depending on the platform, the python interpreter (CPython version but also PyPy, Cython, Jython, etc.), because multiple assignments is not meant to be concurrency safe.

So I'm ok to remove started, but then just to be on the safe side let's set start_t in its separate assignment with a clear comment:

self.last_print_t = self._time()
self.start_t = self.last_print_t  # define start_t last to avoid racing conditions

[lrq3000]

No Casper, I disagree with this PR, you can't just merge your own PR whenever you want like this. Are we dropping the review system?

Anyway I will remove this merge, it should be:

self.last_print_t = self._time()
self.start_t = self.last_print_t  # define start_t last to avoid racing conditions

[casperdcl]

oh hi @lrq3000 didn't see your message here from 2 days ago. happy with the correction, thanks.