-
Notifications
You must be signed in to change notification settings - Fork 36
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
perf: make module analysis async #477
perf: make module analysis async #477
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code in here is pretty crazy, but it was initially crazy. We should spend some time cleaning this up in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you jot down some notes in an issue about what's crazy and how it could be improved? This might be a good task in the future for someone getting to know this codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By "crazy" I just mean messy. Someone needs to go through and clean it up sometime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, feels like it should be split into multiple smaller modules.
maybe this should rather be "perf: make module analysis async" |
pub(crate) enum ModuleSourceAndInfo { | ||
Json { | ||
specifier: ModuleSpecifier, | ||
source: Arc<str>, | ||
}, | ||
Js { | ||
specifier: ModuleSpecifier, | ||
media_type: MediaType, | ||
source: Arc<str>, | ||
maybe_headers: Option<HashMap<String, String>>, | ||
module_info: Box<ModuleInfo>, | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised you don't get a warning about enum variants big size difference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, feels like it should be split into multiple smaller modules.
This allows us to parse source files in parallel for the first run:
Same speed on second run:
Closes #474