Skip to content
This repository has been archived by the owner on Mar 23, 2023. It is now read-only.

unsafe: Removes most of the unsafe casts. #58

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nairb774
Copy link
Contributor

@nairb774 nairb774 commented Jan 6, 2017

Rather than forcing casts from an *Object to the desired type, we store a "self" reference on the object to allow us to get back to the containing type. This does increase the amount of ram each object takes up (~2 more pointers), the chance of miscasting code goes down dramatically.

There is still a bad cast resulting from values coming out of WrapNative, and this has helped corner that cast. The actual fix is quite involved, but this should prevent similar problems in the future (hopefully).

I expect that this might be a little controversial, but the WrapNative bug I am trying to pin down is the result of the following:

type foo uint64
WrapNative(foo(MaxInt+100))

The returned value is a memory mashup of an Int and a Long. There is a similar test already, but it only uses a raw uint64 rather than something like foo above.

Thoughts?

Rather than forcing casts from an *Object to the desired type, we store a "self"
reference on the object to allow us to get back to the containing type. This
does increase the amount of ram each object takes up (~2 more pointers), the
chance of mis-casting code goes down dramatically.

There is still a bad cast resulting from values coming out of WrapNative, and
this has helped corner that cast. The actual fix is quite involved, but this
should prevent similar problems in the future (hopefully).
@trotterdylan
Copy link
Contributor

Thanks for the PR! I definitely like the idea of making the code safer. I have a few concerns:

  1. As you mentioned, the additional size of the self pointer.
  2. Additional cost during object creation of initializing self? This one I'm less sure of. I know that interfaces generally are expensive because they often require an allocation. In this case, the interface is part of the struct so maybe there's no additional allocation in creating this interface?
  3. The casts now become type assertions which I think are quite a bit more expensive than unsafe casts. Again, not too sure of this one.

It'd be worth running the benchmarks to get a sense of some of these costs.

@nairb774
Copy link
Contributor Author

Got a little bogged down at work. I think your concern about the performance is worth keeping an eye on. Unfortunately I made a bunch of aggressive changes in this which likely should be pulled apart and measured separately. Roughly I was seeing a 15% impact in performance on the python benchmark suite. The upside is I think I can recover some (more? unmeasured right now) by generating code that is easier for the go compiler to optimize.

Regardless, in summary, I am going to split this up into smaller parts and get each part measured and we can look at them that way.

@alanjds
Copy link
Contributor

alanjds commented Oct 6, 2018

@nairb774 have you some news on this? Or some pointers if anyone wants to continue your work?

I migrated your issue to grumpyhome#81 (forked), where development is still ongoing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants