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

Enhance Job Queue Implementation with Priority-Based Reordering and SwiftDocs Annotations #276

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

admirsaheta
Copy link

@admirsaheta admirsaheta commented Nov 29, 2024

This is an update for the #275 Pull Request

This PR introduces the following updates to the job queue implementation in the JavaScriptEventLoop:

1. Priority-Based Job Queue Enhancements

  • Refactored the queue to support job insertion and execution based on priority.
  • Introduced a queueLock to handle concurrent job insertion safely.
  • Improved job execution with clear separation of concerns between inserting, claiming, and running jobs.

2. Documentation Improvements

  • Added concise SwiftDoc-style comments for all public and private functions.
  • Provided parameter and return annotations to improve code readability and maintainability for future developers.

3. Code Structure Improvements

  • Simplified assertions and locking mechanisms for better thread safety.
  • Improved handling of the UnownedJob with better encapsulation and type safety.

Checked all Github Actions prior on my fork here

@kateinoigakukun
Copy link
Member

I have a few comments:

  1. Foundation dependency:
    We aim to avoid depending on Foundation in this library, as it tends to increase binary size significantly.

  2. Queue priority support?:
    You said

    Refactored the queue to support job insertion and execution based on priority.

    However it seems the current implementation already supports job insertion and execution based on priority, though the header comment might be misleading.

  3. Concurrent access lock:

    Introduced a queueLock to handle concurrent job insertion safely.

    Sorry, I think the implementation does not protect anything as queueLock creates a new NSLock instance everytime. Could you please test concurrent access scenarios?

Finally, as this module is a critical part of the concurrency support in WebAssembly, we generally prefer to make changes only if they provide clear benefits, such as fixing bugs or improving performance. For other types of changes, we may be less likely to merge them, but we truly appreciate your contribution and effort.

@admirsaheta
Copy link
Author

I understand the concern regarding the dependency on Foundation. I’ll work on removing or replacing the Foundation dependency to reduce the binary size. My goal is to refactor it in a way that is still efficient and works well in the WebAssembly context without adding unnecessary overhead.

I’ll make these changes in a day or two and will ensure that the code performs well in this environment. I’ll keep the PR open and continue to iterate on it based on your feedback.

Thanks again for your review, and I appreciate your patience. I’m looking forward to getting this working in the most efficient way possible!

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