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

Add async methods to module trace #2

Merged
merged 2 commits into from
Sep 12, 2024
Merged

Conversation

lgxbslgx
Copy link

@lgxbslgx lgxbslgx commented Sep 9, 2024

This patch adds two async methods to module trace which can be used by project moon. See moonbitlang/moon#276 for more information.

Copy link

From the git diff output provided, here are three observations that might be worth considering:

  1. Unused Import:

    +use std::future::Future;

    The import of std::future::Future is added but does not appear to be used anywhere else in the file. This import can be removed to avoid clutter and potential confusion.

  2. Potential Unsafe Mutation:

    #[inline]
    #[allow(static_mut_refs)]
    pub async fn async_scope<T>(name: &'static str, f: impl Future<Output = T>) -> T {
        // Safety: accessing global mut, not threadsafe.
        unsafe {
            match &mut TRACE {
                None =>  f.await,
                Some(t) => t.async_scope(name, f).await,
            }
        }
    }

    The function async_scope accesses a mutable static variable TRACE in an unsafe block. This is potentially not thread-safe and could lead to race conditions if this function is called concurrently from different threads. Consider using thread-local storage or another mechanism to ensure safe concurrent access.

  3. Async Scope Overhead:

    async fn async_scope<T>(&mut self, name: &str, f: impl Future<Output = T>) -> T {
        let start = Instant::now();
        let result = f.await;
        let end = Instant::now();
        self.write_complete(name, 0, start, end);
        result
    }

    The async_scope function measures the time taken by the future f but adds overhead by awaiting the future within the function. If f is a lightweight future, the timing might not accurately reflect the actual time taken by f due to the overhead of the async context switch and timing measurement itself. Consider if the timing is critical and if there might be a more accurate way to measure the time without the overhead of awaiting within the function.

These observations aim to help improve code quality and address potential issues that might impact the performance, safety, or clarity of the code.

Copy link
Collaborator

@Young-Flash Young-Flash left a comment

Choose a reason for hiding this comment

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

Thanks!

@Young-Flash Young-Flash merged commit 676a840 into moonbitlang:main Sep 12, 2024
2 checks passed
Young-Flash pushed a commit that referenced this pull request Sep 12, 2024
* Add async methods.

* Format.
@lgxbslgx lgxbslgx deleted the ASYNC branch September 12, 2024 08:15
Young-Flash pushed a commit that referenced this pull request Nov 11, 2024
* Add async methods.

* Format.
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