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

Use SafeNumeric to protect max_index in ImportThreadData #85295

Conversation

jsjtxietian
Copy link
Contributor

@jsjtxietian jsjtxietian commented Nov 24, 2023

When trying to debug #85237 , @TheDuriel and I found a issue in threaded reimport function as the title says. It still did not solve the problem because it's not related to the "correctness" of importing, it affects the progress bar UI.

The real cause of #85237 still needs more investigation.

@jsjtxietian jsjtxietian force-pushed the use-mutex-protect-max_index-in-ImportThreadData branch from 72c0f2a to 6b15ad4 Compare November 24, 2023 07:18
@Chaosus Chaosus added this to the 4.3 milestone Nov 24, 2023
@jsjtxietian jsjtxietian force-pushed the use-mutex-protect-max_index-in-ImportThreadData branch from 6b15ad4 to 886cfd8 Compare November 27, 2023 04:28
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

The thread safety procedure itself looks good to me, not familiar with the larger structure so will need some more eyes on it

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.

I think this is a good use case for SafeNumeric and its exchange_if_greater() function.

@jsjtxietian jsjtxietian force-pushed the use-mutex-protect-max_index-in-ImportThreadData branch from 886cfd8 to 4861ab4 Compare November 28, 2023 04:52
@API-Beast
Copy link

@RandomShaper
^ OP pushed the requested change.

@akien-mga akien-mga changed the title Use mutex to protect max_index in ImportThreadData Use mutex to protect max_index in ImportThreadData Jan 4, 2024
Copy link
Contributor

@lyuma lyuma left a comment

Choose a reason for hiding this comment

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

Looks good to me. This avoids a data race in the progress bar.

(and generally speaking, data races can lead to a wide range of undefined behavior which could be far outside the scope of just the progress bar, so it is good to fix this)

@lyuma lyuma dismissed RandomShaper’s stale review May 2, 2024 06:07

The change was implemented: This code now uses SafeNumeric<> and exchange_if_greater as requested.

@lyuma
Copy link
Contributor

lyuma commented May 2, 2024

I would rather merge this rather than the omnibus PR #88122 because it avoids getting bogged down by finding all the people to review bugfixes in three completely separate parts of the engine... and the StringName one risks a serious performance impact.
@RandomShaper if you would like to review the SafeNumeric part, but I think it's pretty straightforward

@AThousandShips AThousandShips changed the title Use mutex to protect max_index in ImportThreadData Use SafeNumeric to protect max_index in ImportThreadData May 2, 2024
@RandomShaper
Copy link
Member

Being here again, after quite a long time, I realize this could be more accurate (and simpler), by, instead of keeping track of the highest index processed, just having an atomic counter and incrementing it by one from each thread.

The current approach is over-optimistic in that, if item 10 has started processing, 0-9 are assumed to have started, despite scheduling may have caused 10 to start before, say, 5.

Also, shouldn't the counter, either in this PR or in my suggestion, be incremented after processing the item?

This PR already improves things, but giving this as food for thought and possible incremental enhancement.

@akien-mga akien-mga merged commit a21824b into godotengine:master May 2, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@jsjtxietian jsjtxietian deleted the use-mutex-protect-max_index-in-ImportThreadData branch May 3, 2024 12:36
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.

7 participants