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

Fix unsafe edge cases for in-place update of records #8539

Merged

Conversation

bjorng
Copy link
Contributor

@bjorng bjorng commented Jun 3, 2024

No description provided.

bjorng added 2 commits May 31, 2024 11:57
The JIT could do unsafe in-place updates of records in
relatively rare circumstances.

As well as correcting the bug, also add the test cases that should
have been there in the first place.
When GC was temporarily disabled (for example during the execution of
some BIFs), an unsafe in-place update of a record could occur.
@bjorng bjorng added team:VM Assigned to OTP team VM fix labels Jun 3, 2024
@bjorng bjorng requested review from garazdawi and jhogberg June 3, 2024 05:49
@bjorng bjorng self-assigned this Jun 3, 2024
Copy link
Contributor

github-actions bot commented Jun 3, 2024

CT Test Results

    3 files    143 suites   50m 20s ⏱️
1 589 tests 1 540 ✅ 49 💤 0 ❌
2 328 runs  2 254 ✅ 74 💤 0 ❌

Results for commit a180597.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@bjorng bjorng merged commit f5bd44e into erlang:maint Jun 4, 2024
18 checks passed
@alexandrejbr
Copy link

@bjorng we have noticed a performance degradation (execution time) after this change. I don't have any more information than this at the moment.

Since I don't have yet any more information at the moment I just want to ask if you'd be expecting that a performance degradation would occur after this change?

@bjorng
Copy link
Contributor Author

bjorng commented Aug 20, 2024

Yes, a performance degradation can occur if the change disabled an unsafe in-place tuple update in your code.

@alexandrejbr
Copy link

@bjorng thanks! We must be doing some kind of anti pattern because for certain test cases we are seeing 36% execution time increase with OTP 27.0.1 compared to the -12% gain we had when we switched to OTP 27.0.

@alx242
Copy link

alx242 commented Aug 26, 2024

@bjorng I am trying to see if any such thing is happening in our code atm but I have hard time detecting it. Do you happen to have a simple method to detect the disabling of an unsafe in-place tuple update?

@bjorng
Copy link
Contributor Author

bjorng commented Aug 26, 2024

There is a change in the compiler in Erlang/OTP 27.0.1 that could potentially explain the slowdown: 28d0a97.

If you have re-compiled your source code with Erlang/OTP 27.0.1, it is possible that some record update operations are no longer optimized. If you have not re-compiled your source, the change in the compiler is not responsible.

To check whether a record update operation uses the in-place optimization, compile using the -S option. Examine the .S files. The first operand for an update_record instruction is {atom,inplace} if operation could be safe to do in-place.

@alx242
Copy link

alx242 commented Aug 26, 2024

I have recompiled my code with the fix from 5dccb45 and with it the performance degrades a bit. If I revert that commit my code runs faster.

I checked the assembly listings of my module that is affected and I did see a few locations with update_record that have {atom,inplace} instruction. However the generated assembly listing looks the same with or without the commit mentioned above.

So can the above mentioned commit impact performance even if the assembly listing becomes the same?

@alx242
Copy link

alx242 commented Aug 26, 2024

Sorry I mentioned the wrong commit. Rather it is this commit that made my performance degrade: a180597.

And I can notice it by just recompiling OTP and not my own code.

@alx242
Copy link

alx242 commented Aug 26, 2024

@bjorng Specifically this line in erl_gc.c seems to be the culprit of the performance change:

p->high_water = (Eterm *) (Uint) -1;

If I comment this out and recompile OTP I can get back the loss. However I haven't got a clue how this relates to my code but would like to know how it can be investigated :)

@alx242
Copy link

alx242 commented Aug 27, 2024

FYI! I tried adding this line: p->high_water = (Eterm *) (Uint) -1; in erts/emulator/beam/erl_gc.c on an OTP-25 version instead and noticed the degradation there as well with my code.

The commit mentions JIT which I'm not using so it feels like it has impact outside of that.

bjorng added a commit to bjorng/otp that referenced this pull request Aug 27, 2024
a180597 in erlang#8539, a correction for an unsafe in-place update
of a record, had the side effect of causing a major (fullsweep)
garbage collection when GC has been temporarily disabled and then
again enabled. Since many BIFs, such a `term_to_binary/1` and
`iolist_to_binary/1, may temporarily disable GC, this could lead to
noticeable performance regressions.

This commit ensures that it is again possible to do a minor collection
when GC is being enabled.
bjorng added a commit to bjorng/otp that referenced this pull request Aug 27, 2024
a180597 in erlang#8539, a correction for an unsafe in-place update
of a record, had the side effect of causing a major (fullsweep)
garbage collection when GC has been temporarily disabled and then
again enabled. Since many BIFs, such a `term_to_binary/1` and
`iolist_to_binary/1`, may temporarily disable GC, this could lead to
noticeable performance regressions.

This commit ensures that it is again possible to do a minor collection
when GC is being enabled.
bjorng added a commit to bjorng/otp that referenced this pull request Aug 27, 2024
a180597 in erlang#8539, a correction for an unsafe in-place update
of a record, had the side effect of causing a major (fullsweep)
garbage collection when GC has been temporarily disabled and then
again enabled. Since many BIFs, such as `term_to_binary/1` and
`iolist_to_binary/1`, may temporarily disable GC, this could lead to
noticeable performance regressions.

This commit ensures that it is again possible to do a minor collection
when GC is being enabled.
@bjorng
Copy link
Contributor Author

bjorng commented Aug 27, 2024

Sorry I mentioned the wrong commit. Rather it is this commit that made my performance degrade: a180597.

That's make more sense to me. I expect that #8751 should fix it. Please try it.

The commit mentions JIT which I'm not using so it feels like it has impact outside of that.

Yes, it has.

@alx242
Copy link

alx242 commented Aug 27, 2024

Will try it 👍

@alx242
Copy link

alx242 commented Aug 27, 2024

My performance issues disappeared :-D great!

bjorng added a commit to bjorng/otp that referenced this pull request Aug 27, 2024
a180597 in erlang#8539, a correction for an unsafe in-place update
of a record, had the side effect of causing a major (fullsweep)
garbage collection when GC has been temporarily disabled and then
again enabled. Since many BIFs, such as `term_to_binary/1` and
`iolist_to_binary/1`, may temporarily disable GC, this could lead to
noticeable performance regressions.

This commit ensures that it is again possible to do a minor collection
when GC is being enabled.
bjorng added a commit to bjorng/otp that referenced this pull request Aug 27, 2024
a180597 in erlang#8539, a correction for an unsafe in-place update
of a record, had the side effect of causing a major (fullsweep)
garbage collection when GC has been temporarily disabled and then
again enabled. Since many BIFs, such as `term_to_binary/1` and
`iolist_to_binary/1`, may temporarily disable GC, this could lead to
noticeable performance regressions.

This commit ensures that it is again possible to do a minor collection
when GC is being enabled.
@bjorng
Copy link
Contributor Author

bjorng commented Aug 27, 2024

Thanks for reporting this performance regression!

@alx242
Copy link

alx242 commented Aug 27, 2024

Thank you for fixing it :)

bjorng added a commit to bjorng/otp that referenced this pull request Aug 28, 2024
a180597 in erlang#8539, a correction for an unsafe in-place update
of a record, had the side effect of causing a major (fullsweep)
garbage collection when GC has been temporarily disabled and then
again enabled. Since many BIFs, such as `term_to_binary/1` and
`iolist_to_binary/1`, may temporarily disable GC, this could lead to
noticeable performance regressions.

This commit ensures that it is again possible to do a minor collection
when GC is being enabled.
bjorng added a commit to bjorng/otp that referenced this pull request Sep 2, 2024
a180597 in erlang#8539, a correction for an unsafe in-place update
of a record, had the side effect of causing a major (fullsweep)
garbage collection when GC has been temporarily disabled and then
again enabled. Since many BIFs, such as `term_to_binary/1` and
`iolist_to_binary/1`, may temporarily disable GC, this could lead to
noticeable performance regressions.

This commit ensures that it is again possible to do a minor collection
when GC is being enabled.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix team:VM Assigned to OTP team VM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants