Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove StateBase vtables #8

Closed
Smattr opened this issue Feb 27, 2018 · 1 comment
Closed

Remove StateBase vtables #8

Smattr opened this issue Feb 27, 2018 · 1 comment

Comments

@Smattr
Copy link
Owner

Smattr commented Feb 27, 2018

Quoting something from the current sources:

          /* FIXME: This is somewhat awkward. We really want to just do
           * `s[slot] = *t` here. However, we got the memory backing `s` from
           * `calloc` so the vtables of the states were never initialised (to
           * non-null). Of course the assignment operator doesn't write to the
           * vtables because it thinks they've already been initialized. The
           * result is that using the assignment operator here appears to work
           * fine but then later operations that access the vtable segfault. As
           * you can imagine, this was not fun to debug. For now we call the
           * copy constructor to ensure we initialise the vtable, but I think a
           * better long term solution is to make StateBase non-virtual and
           * hence remove its vtable. To do this, we need something else to
           * inherit from BitBlock in place of StateBase and it in turn
           * reference a StateBase.
           */

Having StateBase participate in a class hierarchy is causing problems. Specifically, I didn't anticipate that all virtual calls cannot be collapsed at compile-time (e.g. a parameter to a function that is a reference to an integer that is not known to be in the state vector or in a local variable). Apart from the problem described above, it is highly undesirable for StateBase to have a vtable for allocation reasons. Checking can allocate millions or billions of these and the vtables add up.

We should refactor this area to pare back StateBase to something closer to a C struct with a few helpers.

Smattr added a commit that referenced this issue Mar 1, 2018
This is little more kludgey as we have to call model.size_bits() outside of
Model itself, but this will enable us to simplify a significant amount of
templated code going forwards.

Related to Github #8 "Remove StateBase vtables"
and Github #10 "why is ArrayValue a BitBlock?"
Smattr added a commit that referenced this issue Mar 1, 2018
We no longer rely on this class and don't need it going forwards. In theory,
this should remove StateBase's vtable but we need some further experimentation
to confirm this.

Related to Github #8 "Remove StateBase vtables"
Smattr added a commit that referenced this issue Mar 1, 2018
This finally achieves our goal of reducing StateBase to a vtable-less POD.

Closes Github #8 "Remove StateBase vtables"
@Smattr
Copy link
Owner Author

Smattr commented Mar 1, 2018

Closed in eeb0e2b.

@Smattr Smattr closed this as completed Mar 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant