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

struct Rav1dFrameContext_task_thread: Replace atomic intrinsics #672

Merged
merged 2 commits into from
Jan 18, 2024

Conversation

rinon
Copy link
Collaborator

@rinon rinon commented Jan 10, 2024

No description provided.

@rinon rinon requested a review from kkysen January 10, 2024 21:48
@kkysen kkysen changed the title Replaced Rav1dFrameContext_task_thread atomics struct Rav1dFrameContext_task_thread: Replace atomic intrinsics Jan 10, 2024
src/internal.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

LGTM besides that indentation typo.

@rinon rinon force-pushed the sjc/rav1d_worker_task-cleanup branch from 0a93786 to 7509587 Compare January 11, 2024 17:41
@rinon rinon force-pushed the sjc/fc-task_thread-atomics branch 2 times, most recently from 3ac1fd4 to edc0f88 Compare January 11, 2024 17:54
@rinon rinon requested a review from kkysen January 11, 2024 17:54
@rinon
Copy link
Collaborator Author

rinon commented Jan 11, 2024

Added another atomic_i32 -> AtomicI32 commit, so worth a quick additional look.

@rinon rinon force-pushed the sjc/fc-task_thread-atomics branch 2 times, most recently from e4c66b9 to eae8a7b Compare January 11, 2024 18:14
Copy link
Collaborator

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

Still looks good with the pending_tasks_merge change now, but what was the hoist from Rav1dFrameContext_task_thread_pending_tasks to Rav1dFrameContext_task_thread for, (for some mutability reasons?)? Can you add that to the PR description or commit message?

@rinon
Copy link
Collaborator Author

rinon commented Jan 11, 2024

Updated the commit message. I moved it out in preparation for making the pending_tasks protected by a Mutex. The merge field was accessed outside the critical section (and it's atomic, so that's fine), so I needed to move it out of the struct that I planned to put into a Mutex.

@rinon rinon force-pushed the sjc/rav1d_worker_task-cleanup branch from 7509587 to ef6b864 Compare January 12, 2024 01:36
@rinon rinon force-pushed the sjc/rav1d_worker_task-cleanup branch from ef6b864 to 4b3f2a2 Compare January 12, 2024 21:09
@rinon rinon force-pushed the sjc/rav1d_worker_task-cleanup branch from 4b3f2a2 to d28be39 Compare January 17, 2024 21:09
Base automatically changed from sjc/rav1d_worker_task-cleanup to main January 17, 2024 21:32
This change moves the `merge` field out of
`Rav1dFrameContext_task_thread_pending_tasks` into the parent struct,
`Rav1dFrameContext_task_thread`, in preparation to move the
`pending_tasks` field into a Mutex.
@rinon rinon merged commit 5e8f5c7 into main Jan 18, 2024
34 checks passed
@rinon rinon deleted the sjc/fc-task_thread-atomics branch January 18, 2024 02:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants