KarthikRIyer / swiftplot

Swift library for Data Visualization :bar_chart:
Apache License 2.0
400 stars 39 forks source link

Convert the plots to value types #82

Closed karwa closed 4 years ago

karwa commented 4 years ago

Hopefully this PR will not be too disruptive for GCI participants. AFAIK none of the GCI tasks involve large changes to these parts of the code, so it should be relatively simple for participants to follow along. These changes might even be easier to understand.

This PR does 2 things:

Stop storing intermediate calculations in plot objects.

We have had bugs where we forgot to clear variables between renders. Rather than store per-render specific things like scaled values in the plots themselves, add a helper DrawingData struct for the plot to stash things in. HasGraphLayout automatically forwards it.

Make the plot objects value types.

To enforce that we didn't forget to clear any per-render specific state, make the plots value types. Now, if we saved some calculations in layoutData(...), the compiler would complain that the function has to be mutating. This means plots now need to be created with var, rather than let.

This also allows us to do things like lay-out and rendering plots on background threads, as each plot (really a "plot description") is now a fully-independent value.

These changes are output-neutral - i.e. no changes to reference images are required.

WilliamHYZhang commented 4 years ago

I wonder if these changes would potentially solve the issues I was facing in #81?

KarthikRIyer commented 4 years ago

Thanks for the PR @karwa. I agree, it shouldn't be too disruptive and they should be able to follow the changes. Merging.

If we plan to render on a background thread, where would we get stuck if each plot would've been a class? Even if each plot wasn't completely independent, they didn't have any static/global variables, did they? And afaik in C++ separate stack memory is allocated for each thread. Am I wrong and is it different in Swift?

karwa commented 4 years ago

If we plan to render on a background thread, where would we get stuck if each plot would've been a class? Even if each plot wasn't completely independent, they didn't have any static/global variables, did they? And afaik in C++ separate stack memory is allocated for each thread. Am I wrong and is it different in Swift?

Classes tend to be allocated on the heap (like the C++ new operator), and you can't force stack allocation in Swift. If you took a plot and captured it in a DispatchQueue.async { ... } block, you only capture a reference to the instance. So somebody else on another thread/queue can mutate the plot's properties while another thread/queue is using them. That can be bad - maybe we are calculating the scaled values on one thread, and another thread is changing the data points simultaneously.

If a class instance has a value-type property (e.g. an Array), then even if there are multiple references to the instance, mutations do not trigger copy-on-write. That's because it is the instance itself that holds the reference to the Array; it doesn't matter if we captured multiple references to that instance or not, the Array itself would still be uniquely-referenced.

Value types also mean that the plots are treated like single "units". That means we can easily detect when any part of a plot has changed by adding a didSet handler. That's handy for making sure that UI-views redraw when the data changes.

var graph: BarGraph<Float, Float> {
  didSet { flagNeedsRedraw() }
}

This doesn't happen for classes, because the instance doesn't change for in-place mutations. Only reassignment (view.graph = BarGraph(...)) causes didSet handlers to be invoked for reference types.