Skip to content

[Proxying][NFC] Move deferred cleanup logic to em_task_queue - #18851

Merged
tlively merged 1 commit into
mainfrom
proxying-zombie-tasks
Feb 28, 2023
Merged

[Proxying][NFC] Move deferred cleanup logic to em_task_queue#18851
tlively merged 1 commit into
mainfrom
proxying-zombie-tasks

Conversation

@tlively

@tlively tlively commented Feb 25, 2023

Copy link
Copy Markdown
Member

We previously had a scheme to defer freeing of em_proyxing_queues until there
were no more outstanding references to their em_task_queues sitting in
message queues. Since the notification messages only ever contain references to
em_task_queues and not the em_proxying_queues, move the deferred cleanup
logic to the em_task_queue layer from the em_proxying_queue layer. This
slightly simplifies the code and is a cleaner separation of concerns.

This is NFC as far as users are concerned, and the code is moved with only two
changes to internal behavior. First, we now use a trylock when culling zombies
to avoid blocking when multiple threads are creating em_task_queues at the
same time. Second, we now enqueue zombie queues at the tail of the zombie list
instead of the head because FIFO behavior seems fairer.

The test for the zombie culling behavior is necessarily less precise now because
it the objects with deferred cleanup are no longer directly user-visible. The
test still does its job, though.

We previously had a scheme to defer freeing of `em_proyxing_queue`s until there
were no more outstanding references to their `em_task_queues` sitting in
message queues. Since the notification messages only ever contain references to
`em_task_queue`s and not the `em_proxying_queue`s, move the deferred cleanup
logic to the `em_task_queue` layer from the `em_proxying_queue` layer. This
slightly simplifies the code and is a cleaner separation of concerns.

This is NFC as far as users are concerned, and the code is moved with only two
changes to internal behavior. First, we now use a trylock when culling zombies
to avoid blocking when multiple threads are creating `em_task_queue`s at the
same time. Second, we now enqueue zombie queues at the tail of the zombie list
instead of the head because FIFO behavior seems fairer.

The test for the zombie culling behavior is necessarily less precise now because
it the objects with deferred cleanup are no longer directly user-visible. The
test still does its job, though.
break;
}
}
frees++;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Does hit not risk counting other misc allocations? (I guess if it ever does this test will just start failing or flaking)..

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, right now this doesn't seem to be a problem, but I could imagine that this test would start flaking in the future. At some point we might want to remove it entirely in favor of a test that builds and tests the internal em_task_queue implementation directly.

@tlively
tlively merged commit ad1dfd7 into main Feb 28, 2023
@tlively
tlively deleted the proxying-zombie-tasks branch February 28, 2023 23:25
impact-maker pushed a commit to impact-maker/emscripten that referenced this pull request Mar 17, 2023
…ipten-core#18851)

We previously had a scheme to defer freeing of `em_proyxing_queue`s until there
were no more outstanding references to their `em_task_queues` sitting in
message queues. Since the notification messages only ever contain references to
`em_task_queue`s and not the `em_proxying_queue`s, move the deferred cleanup
logic to the `em_task_queue` layer from the `em_proxying_queue` layer. This
slightly simplifies the code and is a cleaner separation of concerns.

This is NFC as far as users are concerned, and the code is moved with only two
changes to internal behavior. First, we now use a trylock when culling zombies
to avoid blocking when multiple threads are creating `em_task_queue`s at the
same time. Second, we now enqueue zombie queues at the tail of the zombie list
instead of the head because FIFO behavior seems fairer.

The test for the zombie culling behavior is necessarily less precise now because
it the objects with deferred cleanup are no longer directly user-visible. The
test still does its job, though.
impact-maker pushed a commit to impact-maker/emscripten that referenced this pull request Mar 17, 2023
…ipten-core#18851)

We previously had a scheme to defer freeing of `em_proyxing_queue`s until there
were no more outstanding references to their `em_task_queues` sitting in
message queues. Since the notification messages only ever contain references to
`em_task_queue`s and not the `em_proxying_queue`s, move the deferred cleanup
logic to the `em_task_queue` layer from the `em_proxying_queue` layer. This
slightly simplifies the code and is a cleaner separation of concerns.

This is NFC as far as users are concerned, and the code is moved with only two
changes to internal behavior. First, we now use a trylock when culling zombies
to avoid blocking when multiple threads are creating `em_task_queue`s at the
same time. Second, we now enqueue zombie queues at the tail of the zombie list
instead of the head because FIFO behavior seems fairer.

The test for the zombie culling behavior is necessarily less precise now because
it the objects with deferred cleanup are no longer directly user-visible. The
test still does its job, though.
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