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

unsafe: Removes most of the unsafe casts. #81

Open
alanjds opened this issue Oct 6, 2018 · 2 comments
Open

unsafe: Removes most of the unsafe casts. #81

alanjds opened this issue Oct 6, 2018 · 2 comments
Labels
imported PR Pull Request imported from google/grumpy waiting feedback Waiting confirmation of changes or fixes

Comments

@alanjds
Copy link

alanjds commented Oct 6, 2018

google#58 opened on Jan 6, 2017 by @nairb774

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?

@alanjds
Copy link
Author

alanjds commented Oct 6, 2018

Comment by trotterdylan
Saturday Jan 07, 2017 at 00:11 GMT


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.

@alanjds
Copy link
Author

alanjds commented Oct 6, 2018

Comment by nairb774
Saturday Jan 14, 2017 at 20:48 GMT


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 alanjds added the imported PR Pull Request imported from google/grumpy label Oct 6, 2018
@alanjds alanjds added the waiting feedback Waiting confirmation of changes or fixes label Oct 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
imported PR Pull Request imported from google/grumpy waiting feedback Waiting confirmation of changes or fixes
Projects
None yet
Development

No branches or pull requests

1 participant