1. Team SimPy
  2. simpy
  3. simpy
  4. Pull requests

Pull requests

#35 Merged
Repository
simpy-improve-condition
Branch
default
Repository
simpy
Branch
default

Improve performance of conditions

Author
  1. luensdorf
Reviewers
Description

As Georgios pointed out on the mailinglist conditions have a bad performance.

It looks like OrderedDict is the culprit, whose instanciation sadly seems to be very expensive. I introduced a custom object for condition values, which mimics a dictionary and is a drop-in replacement as far as the tests are concerned.

The benchmark Georgios added to his mail takes 6.76 seconds on my machine. This changeset drops the runtime to 2.82 seconds, which should be enough to outpace SimPy 2.

This is one of many solutions. The result of a condition may also be a list of (event, value) tuples, which is easily convertible to a dict or the list of values only with a little bit more work on the user-side. I would be fine with that solution too, but it will of course break the API.

Comments (13)

  1. Stefan Scherfke

    Without having taken a look at your code – the performance improvement looks nice and and a list of tuples as a replacement for the results dict also sounds nice. I often had the use case "I’d like the value of the first result“ wich would now be results[0][1] – no need to store the event in a separate name just to exit its value. :-)

        1. luensdorf author

          So you want to break backwards compatability?

          There's also a problem with using lists, we either sacrify dictionary access or value expansion. If we use list of (event, value) tuples, the user can easily convert it to a dictionary dict(results). This will make expansion difficult, e.g. a, b, c = list(zip(*(yield env.all_of(events)))

          The other way around, using a tuple (events, values) simplifies expansion, but make conversion to a dict cumbersome.

          1. Stefan Scherfke

            If we removed OrderedDict, SimPy would run on Python 2.6 again (you know why this could be useful for me ;-)). If the new semantic was

            results = yield env.all_of([evt1, evt2])
            results == [(evt1, val1), (evt2, val2)]
            

            both, list expansion and converting to dict, would work nicely:

            ev1, ev2 = results
            ev1 == (evt1, val1)
            ev2 == (evt2, val2)
            
            dict(results) == {evt1: val1, evt2: val1}
            
                1. luensdorf author

                  No, I meant the expansion of the values. Currently it can be done like this:

                  a, b, c = (yield env.all_of(events)).values()
                  

                  But this would become:

                  a, b, c = list(zip(*(yield env.all_of(events)))[1]
                  
                    1. Stefan Scherfke

                      But I’m not sure if this is really the common case.

                      And I think the improved performance and re-gained py26 compatibility may be worth it.

                      Did you take a look how you use cond. events in simpy.io?

                    2. luensdorf author

                      This looks a bit better, but is still a bit too complicated for my taste. For reference, the full line would look like this:

                      _, (a, b, c) = zip(*(yield env.all_of(events)))
                      

                      Also note that the ConditionValue is not using an OrderedDict and would probably work with Python 2.6 too.

                      This pullrequest has nothing to do with simpy.io.