JuliaDynamics / DiscreteEvents.jl

Discrete event generation and simulation in Julia
MIT License
56 stars 10 forks source link

Getting rid of all functionality involving gobal scope #15

Closed non-Jedi closed 4 years ago

non-Jedi commented 4 years ago

Similar to what was discussed in #12, there's some functionality that depends on global scope (specifying events dependent on a global variable given as a symbol, passing an expression instead of a function to create an event must then be evaluated in global scope, etc.). These will in general severely negatively affect performance, and I'm wondering what the advantage is compared to requiring users to explicitly pass variables instead of implicitly passing them through global scope?

It seems like an obvious performance foot-gun to eliminate.

Steps taken

pbayer commented 4 years ago

Yes, I know that it is bad for performance. But simulations often are done in a lab-like manner, where you do quick and dirty experiments in the REPL or in a notebook and performance does not matter much in a first approach.

  1. I agree, that functionality to work with global variables should be included only if necessary or much more convenient than the other provided functions. I will look after it.
  2. I agree further, that the documentation has many examples using global variables without any performance warnings. I can include such warnings. Maybe in order to make this clear, we can do benchmarks and include them in the docs.
pbayer commented 4 years ago
  1. A third way is to advise the user how to declare type stable global variables (e.g. as arrays) if he wants them and cares for performance at the same time.
  2. I will include a performance section in the documentation where such issues can be discussed.
pbayer commented 4 years ago

Introduced a one-time warning in simExec and warnings and deprecation notice in documentation about using evaluation at global scope.

pbayer commented 4 years ago

one nice example, why I find the global scope functionality useful for experimental work is in the house heating example. There I work with global variables and in function switch() I schedule a conditional event with event!(SF(switch, t1, t2), SF(≤, :Tr, t1)) where :Tr is a symbolic reference to room temperature Tr, which changes over time.

Another implementation of the same problem is done in the heating benchmark, which is faster and more refined. There the same conditional event is scheduled with an anonymous function in event!(SF(switch!, h, t1, t2), SF((h, x)-> h.Tr ≤ x, h, t1))

But from a user perspective the first implementation is still good enough and runs in a instant of 0.1 seconds. Now (after the last commits) it gives the user a one-time warning that this is slow. Ok!

The code in clock.jl is not slowed down much by this functionality because everything is handled with function barriers and multiple dispatch. So the cost is only the branching.

Therefore I think we should let it like this until at least the next version (v0.4.0)

edit: 2020-01-03

pbayer commented 4 years ago

!!! Breaking

The macros @val and @tau and the respective functions val and tau, which allowed to compare with global variables have been removed from master.

The functionalities

are still maintained, but give a warning as described above.

non-Jedi commented 4 years ago

Thanks for looking at this @pbayer. My work hasn't involved DES lately, so I haven't been able to return to Simulate.jl and use it much less contribute, but I still hope to do so in the future. Really appreciate you releasing your work on this to the public.

pbayer commented 4 years ago

Thanks for revisiting anyway @non-Jedi . Any feedback, help and contribution is appreciated and helps me to improve things.

pbayer commented 4 years ago

There are two further improvements on master regarding this topic:

  1. Now arbitrary functions or function closures can be scheduled as events besides expressions. With them anything can be easily expressed.
  2. Expressions and Symbols given to event!, periodic! or fun (which has replaced SimFunction) are now only evaluated in Main scope. Any functionality to specify other scopes has been removed. Thus only the end user can use this functionality (if he wants to) but no other module.

See Events and fun in the development documentation.

itsdfish commented 4 years ago

Hi @pbayer -

Thank you for putting this package together. It looks really nice! I'm new to DES and I am going through your source code and examples to learn how DES is implemented. In the process, I noticed several places in types.jl in which abstract containers/fields are used. In some cases, this can cause significant performance issues. For example, in

mutable struct Prc
    id::Any
    task::Union{Task,Nothing}
    clk::Union{AbstractClock,Nothing}
    f::Function
    arg::Tuple
    kw::Base.Iterators.Pairs

    Prc( id, f::Function, arg...; kw...) = new(id, nothing, nothing, f, arg, kw)
end

the fields id, f, and args are abstract (perhaps kw too). One solution might be the following:

mutable struct Prc{F<:Function,ID,T<:Tuple}
    id::ID
    task::Union{Task,Nothing}
    clk::Union{AbstractClock,Nothing}
    f::F
    arg::T
    kw::Base.Iterators.Pairs
end

Perhaps you have tested the performance implications and found that they are negligible. Nonetheless, I have seen large performance hits with abstract fields like Function. So I thought I might bring that to your attention just in case.

pbayer commented 4 years ago

Thank you, this is certainly better. Until now in Simulate.jl tasks aren't started that often, so I didn't look into Prc yet.

itsdfish commented 4 years ago

No problem. I suspect the issue with abstract fields might be more important with SimFunction because those appear to be processed frequently in the package. However, it looks SimFunction might be deprecated because I cannot find it on master.

pbayer commented 4 years ago

Yes, SimFunction migrated into fun, which is a function closure. It is faster and gives the possibility to pass pure functions to events as well. This goes now as T<:Action into DiscreteEvent.

But you have made a point in highlighting that I should pass more concrete types to functions than I do. Thank you again for looking at it.

itsdfish commented 4 years ago

You are very welcome! By the way, is Discourse your preferred forum for asking questions about the package? I will probably have a few questions at some point in the future.

pbayer commented 4 years ago

I'm on discourse. But master has changed a lot since I announced 0.2 on discourse. I plan to release 0.3 until end of march 2020 with multithreading, a interface to resources (maybe faster channels) and a streamlined documentation. If you want ask questions about master until then, may be, it is better to do it here. You are welcome to open issues.

pbayer commented 4 years ago

Now with commit 7c4c0feb60ef5ed9b64e6d0c25c62eda8d1b6418 I added a section on events and variables discussing variables in global scope.

I think the initial problem is mitigated enough. Still the user can do something in global scope if he wants to.

pbayer commented 4 years ago

Now I rewrote the introductory examples without any example using global scope.