Move interruption and all of the safety checks into a new event.

#30 Merged at 12c5834
Repository
simpy-interruption
Branch
default
Repository
simpy
Branch
default
Author
  1. Ontje Lünsdorf
Reviewers
Description

_resume() did always check if the processes state was being messed with by an interrupt, needlessly if this was not the case. This patch moves that responsibility into the interrupt itself.

This patch is a code cleanup but does also introduce a small performance gain (although very close to the error margin):

tip of simpy/simpy

Running benchmarks for rev. None ...
Running simpy3[None].simple_sim(316, 316) ...
... 0.65s
Running simpy3[None].simple_sim(10, 10000) ...
... 0.60s
Running simpy3[None].simple_sim(10000, 10) ...
... 0.78s
Running simpy3[None].wait_for_proc(100, 100) ...
... 0.27s
Running simpy3[None].subscribe(100, 100) ...
... 0.62s
Running simpy3[None].condition_events(10000) ...
... 1.61s
Running simpy3[None].condition_wait(10000) ...
... 2.07s

tip of luensdorf/simpy-interruption

Best of 20 runs:
Running benchmarks for rev. None ...
Running simpy3[None].simple_sim(316, 316) ...
... 0.63s
Running simpy3[None].simple_sim(10, 10000) ...
... 0.58s
Running simpy3[None].simple_sim(10000, 10) ...
... 0.77s
Running simpy3[None].wait_for_proc(100, 100) ...
... 0.27s
Running simpy3[None].subscribe(100, 100) ...
... 0.62s
Running simpy3[None].condition_events(10000) ...
... 1.60s
Running simpy3[None].condition_wait(10000) ...
... 2.07s

Comments (11)

  1. Ontje Lünsdorf author

    I've updated the documentation and while at it, remove some obsolete and duplicate tests.

    Is there anything missing?

    If not could you please tox-test this?

  2. Andreas Beham

    You'll have to move the removal of the target's resume callback in the constructor of Interruption if you plan to integrate pull request 31 in the future.

      1. Andreas Beham

        Because you added code to the interrupt method where the deregistration occurs. You're also missing URGENT so you can have an event that executes before interrupt which resumes the process. I have not tested this to be honest. I just had the feeling that doing this during the callback is too late.

        1. Ontje Lünsdorf author

          If pull request 31 is to be merged, URGENT will not be used anymore for interrupts. But you may be correct if this pull request gets applied to the current tip. In that case, we are missing a test. I will take a look at this in more detail. Thanks for your review :)

        2. Ontje Lünsdorf author

          Hi Andreas,

          I looked at the code again and I think there is no problem. What do you mean with "missing URGENT"? The Interruption is scheduled as urgent (see line 223), so normal events cannot be executed before the interrupt. Only concurrent interrupts may be executed before an interrupt and in this case I think it's correct to deregister the callback right before the process gets resumed.

          1. Andreas Beham

            Hi,

            In pull request 31 URGENT is not used anymore, urgent events are put in a queue, while scheduled events are put in a heap. In that pull request you added the removal of the target's resume callback at the same time the interrupt was created. This would be equivalent to removing the target's resume callback in the initialization of Interruption, rather than in the callback. See lines 279-281 in pull request 31 on events.py.

            I imagine following case:

            def procA():
              with res.request() as req:
                yield req
                yield env.timeout(2)
            
            def procB(proc):
              proc.interrupt()
              yield timeout(1)
            
            env = Environment()
            proc = env.process(procA())
            env.process(procB(proc))
            env.run()
            

            If I am not mistaken you would need to prevent the event req from waking up procA. But the interruption would be enqued after req's success. Therefore Interruption would need to unregister the callback of the process' target as soon as it is created instead of when it is processed.

            But okay, this can be postponed to the point at which pull request 31 is to be merged.