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

GDScript: Lambda hotswap fixes #86569

Merged
merged 1 commit into from
Jan 5, 2024

Conversation

rune-scape
Copy link
Contributor

@rune-scape rune-scape commented Dec 28, 2023

im glad #85248 got merged and fixed my mistakes from #81628
however,
it has some problems that this PR fixes:

  • thread_enter and thread_exit are inadequate for initializing the threadlocal lists, as external thread created by, FMOD for example, arent tracked at all, leading to a segfault when it tries to access the threadlocal list
  • Fix lambda cross-thread dynamics (take 2) #85248 is, no offense, over complicated, manual refcounting is unnecessary and error prone, just keeping track of the lists should be used instead of moving orphaned list elements to the main thread list

and,, another thing fixed by this:

  • my original code was fundamentally broken
  • it nulls lambdas unrelated to the currently reloading script, making hot-reloading lambdas still broken, just safer, no crashing
  • just alot safer, regular mutex and data member arrangement,, easy to wrap head around

crucially it works on my project that uses lambdas and needs hot-reloading to properly update them, where the current master commit fails,, lambdas just become invalid for all but 1 script
keeping only the relevant updateable func ptrs in each script with a mutex for each is enough to make this fast and safe in lambda creation, and script reloading and clearing.
and easier to read and modify the code
it should function as i meant it to originally in #81628 and now waste no time checking unrelated elements when reloading

the following recent related bugs are still gone after this PR:

but i could not reproduce #85151, if someone who can reproduce it can test that its still fixed after this PR i would be so grateful
sorry for the wait

@rune-scape rune-scape requested a review from a team as a code owner December 28, 2023 05:55
@rune-scape
Copy link
Contributor Author

trying to find out why only the linux builds are failing..

@Chaosus Chaosus added this to the 4.3 milestone Dec 28, 2023
@rune-scape rune-scape force-pushed the rune-fix-lambda-hotswap2 branch 3 times, most recently from 1b58b59 to e7948de Compare December 28, 2023 08:49
@rune-scape
Copy link
Contributor Author

rune-scape commented Dec 28, 2023

#0  0x000000000a2119dd in GDScript::UpdatableFuncPtrOwnerListContainer::UpdatableFuncPtrOwnerListContainer (
    this=0x7fffff400d80) at modules/gdscript/gdscript.cpp:1397
1397    GDScript::UpdatableFuncPtrOwnerListContainer::UpdatableFuncPtrOwnerListContainer() {

oops it seems threadlocal var initializers arent re-entrant safe... only on linux,

i rewrote it to be way simpler anyway,

@RandomShaper
Copy link
Member

RandomShaper commented Dec 28, 2023

You should have seen its first iteration. 😆

Now seriously, I did my best to understand the intent of the original code so I could provide a fix. I had to make a few assumptions. I'm happy there's a proper patch now.

@rune-scape
Copy link
Contributor Author

rune-scape commented Dec 28, 2023

@RandomShaper you did a good job, it was pretty bad code,,,
i should have gone with simplest option to begin with :)

modules/gdscript/gdscript_lambda_callable.h Outdated Show resolved Hide resolved
modules/gdscript/gdscript_lambda_callable.h Outdated Show resolved Hide resolved
modules/gdscript/gdscript.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript.cpp Outdated Show resolved Hide resolved
@rune-scape rune-scape force-pushed the rune-fix-lambda-hotswap2 branch 3 times, most recently from 28d9d59 to 7902c58 Compare December 29, 2023 09:23
Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. There are change requests by others, I agree with, though.

modules/gdscript/gdscript.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga merged commit bf1de98 into godotengine:master Jan 5, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@rune-scape
Copy link
Contributor Author

rune-scape commented Jan 5, 2024

i think this should probably be cherry-picked for 4.2 ,, ill make a commit

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

Successfully merging this pull request may close these issues.

5 participants