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

Support concurrent swc_common via feature #250

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

spence
Copy link

@spence spence commented May 17, 2024

When using swc_common with feature = ["concurrent"], deno_ast fails to compile.

error[E0277]: `Rc<RefCell<Vec<swc_common::errors::Diagnostic>>>` cannot be sent between threads safely
   --> .../deno_ast-0.38.2/src/transpiling/mod.rs:324:46
    |
324 | impl crate::swc::common::errors::Emitter for DiagnosticCollector {
    |                                              ^^^^^^^^^^^^^^^^^^^ 
    |    `Rc<RefCell<Vec<swc_common::errors::Diagnostic>>>` cannot be sent between threads safely
    |
    = help: within `DiagnosticCollector`, the trait `Send` is not implemented for 
      `Rc<RefCell<Vec<swc_common::errors::Diagnostic>>>`, which is required by `DiagnosticCollector: Send`

error[E0308]: mismatched types
  --> .../deno_ast-0.38.2/src/emit.rs:79:7
   |
78 |     let mut writer = Box::new(JsWriter::new(
   |                               ------------- arguments to this function are incorrect
79 |       source_map.clone(),
   |       ^^^^^^^^^^^^^^^^^^ expected `Arc<SourceMap>`, found `Rc<SourceMap>`
   |
   = note: expected struct `Arc<swc_common::SourceMap>`
              found struct `Rc<swc_common::SourceMap>`

Resolves #219

cc @dsherret

@spence spence force-pushed the 0.38.2-concurrent branch from f763ed2 to 22bc65f Compare May 17, 2024 18:34
@CLAassistant
Copy link

CLAassistant commented May 17, 2024

CLA assistant check
All committers have signed the CLA.

}

unsafe impl Send for DiagnosticCollector {}
unsafe impl Sync for DiagnosticCollector {}
Copy link
Author

Choose a reason for hiding this comment

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

we can feature gate these, but without concurrent, Send and Sync are noops:

#[cfg(not(feature = "concurrent"))]
mod single {
    pub use once_cell::unsync::{Lazy, OnceCell};
    /// Dummy trait because swc_common is in single thread mode.
    pub trait Send {}
    /// Dummy trait because swc_common is in single thread mode.
    pub trait Sync {}

    impl<T> Send for T where T: ?Sized {}
    impl<T> Sync for T where T: ?Sized {}

    pub use std::{
        cell::{
            Ref as ReadGuard, RefMut as WriteGuard, RefMut as MappedWriteGuard,
            RefMut as LockGuard, RefMut as MappedLockGuard,
        },
        rc::{Rc as Lrc, Weak},
    };
}

Copy link
Member

Choose a reason for hiding this comment

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

If someone is using concurrent then this not being thread safe will cause thread concurrency issues. Probably this will need a thread safe implementation when compiling with concurrent.

@@ -29,6 +29,7 @@ typescript = ["transforms", "swc_ecma_transforms_typescript"]
utils = ["swc_ecma_utils"]
view = ["dprint-swc-ext/view"]
visit = ["swc_ecma_visit", "swc_visit", "swc_visit_macros", "swc_macros_common"]
concurrent = ["swc_common/concurrent"]
Copy link
Member

@dsherret dsherret May 17, 2024

Choose a reason for hiding this comment

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

As mentioned in #219 (comment) , I'm not sure this is a scenario we want to bother to support.

Do you really need this feature? How much faster have you found it? We've just started using separate threads per file. Maybe in bundling scenarios this is faster?

Copy link
Author

@spence spence May 18, 2024

Choose a reason for hiding this comment

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

unfortunately, a number of swc libraries bake-in swc_common's concurrent feature and including any of them prevents deno_ast from compiling:

i'm working from a fork of farm, which uses rayon for concurrent builds, and they benchmark against other bundlers using swc (e.g., rspack).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. We can add a concurrent feature here, but no guarantee it's going to work in the future. It might be removed at any time due to the maintenance cost and you might have to contribute fixes to get it working again.

@spence spence force-pushed the 0.38.2-concurrent branch 2 times, most recently from ed4e2dc to 65651cf Compare August 3, 2024 04:14
@spence spence force-pushed the 0.38.2-concurrent branch from 40bfbc5 to 437ef65 Compare August 3, 2024 04:33
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.

Using deno_ast in a project that contains swc_common -F concurrent breaks the build
3 participants