-
-
Notifications
You must be signed in to change notification settings - Fork 419
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
Fix runtime crash when tracing class iso containing struct val #3993
Conversation
@ponylang/committer I'm finding a 0.7% performance degradation from this fix as is on the performance test machine. Here's the basic command being run in case it means anything to you:
Roughly:
The runs are about 5 minutes (give or take a few nanos) in length. We are using the following runtime options:
I'm going to try some other configurations to see if the same small degradation holds. If pony committers agree this is an acceptable perf lose to fix the bug (I think it is), I'll play around with changing the layout of fields in: typedef struct object_t
{
void* address;
size_t rc;
uint32_t mark;
bool immutable;
pony_type_t* type;
} object_t; to see if changing the location of type makes a perf difference. I doubt it will, but its worth a try. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good, and I agree with the small perf loss being acceptable to fix this crash-safety bug.
I'll look into moving around where type additional field is in object map to see if it has a performance impact. I doubt it will but, it's worth checking and these perf tests are easy to do while watching TV or something. |
Hi @SeanTAllen, The changelog - fixed label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do. Release notes are added by creating a uniquely named file in the The basic format of the release notes (using markdown) should be:
Thanks. |
There's no difference I can see when moving where in objectmap the type is located, so I'm leaving as-is at the end. |
I was storing the type for the wrong object into the object map in acquire_object. This led to ponyup going boom. Other programs would as well, but ponyup exhibited. I'm going to spend some time thinking about how to test this, but at the moment I don't know the specific combo of Pony code that got us here.
Prior to this fix, you could crash the runtime if you sent a
val
struct wrapper in aniso
class. For example:Closes #3840