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

Query glibc version only once to speed up JSTransformer on Linux #9117

Merged
merged 2 commits into from
Jul 3, 2023

Conversation

pastelsky
Copy link
Contributor

@pastelsky pastelsky commented Jul 1, 2023

↪️ Pull Request

While profiling Parcel as it ran on a large app at Atlassian, I noticed that calls to process.report.getReport() stood out in the trace —

Screenshot 2023-07-02 at 1 28 48 AM

This function is in fact called once per transformation, to get the glibc version on linux and on an average takes 10ms per call. However, the version of glibc on a system isn't likely to change during a parcel run. Moving the call out so it is queried only once led to up to a ~35% improvement in overall build times on Linux.

This was tested on a 15 Core c6i AWS box, on Node 16.15.0, with both the SWC and Babel transformers enabled. The % improvement might vary based on individual setups, however, it is still likely to be significant.

@pastelsky pastelsky changed the title [Draft] Query process report only once to speed up JSTransformer Query process report only once to speed up JSTransformer Jul 1, 2023
@pastelsky pastelsky changed the title Query process report only once to speed up JSTransformer Query process report only once to speed up JSTransformer on Linux Jul 1, 2023
@pastelsky pastelsky changed the title Query process report only once to speed up JSTransformer on Linux Query glibc version only once to speed up JSTransformer on Linux Jul 1, 2023

// $FlowFixMe
let {glibcVersionRuntime} = process.report.getReport().header;

async function loadOnMainThreadIfNeeded() {
if (
!isLoadedOnMainThread &&
Copy link
Member

Choose a reason for hiding this comment

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

hmm it's surprising to me that this is running more than once per thread given this check here. any idea why that's happening?

Copy link
Contributor Author

@pastelsky pastelsky Jul 2, 2023

Choose a reason for hiding this comment

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

isLoadedOnMainThread is set to true only if the legacy glibc version condition is met —

if (glibcVersionRuntime && parseFloat(glibcVersionRuntime) <= 2.17) {
let api = WorkerFarm.getWorkerApi();
await api.callMaster({
location: __dirname + '/loadNative.js',
args: [],
});
isLoadedOnMainThread = true;

Else, its called once per transform —

async transform({asset, config, options, logger}) {
let [code, originalMap] = await Promise.all([
asset.getBuffer(),
asset.getMap(),
init,
loadOnMainThreadIfNeeded(),
]);

I think moving the check outside might be better in that case?

    if (glibcVersionRuntime && parseFloat(glibcVersionRuntime) <= 2.17) {
      let api = WorkerFarm.getWorkerApi();
      await api.callMaster({
        location: __dirname + '/loadNative.js',
        args: [],
      });
    }

   isLoadedOnMainThread = true;

Copy link
Member

Choose a reason for hiding this comment

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

Ah right

@pastelsky
Copy link
Contributor Author

Moved the isLoadedOnMainThread = true out instead, which should be a neater way to do this.

@pastelsky
Copy link
Contributor Author

pastelsky commented Jul 2, 2023

@devongovett do you mind approving the CI workflow whenever you can? Curious to see the effect this has on the parcel benchmark suite.

@pastelsky
Copy link
Contributor Author

Looks like the benchmark pipeline has been broken, and performance hasn't been published for a while according to —

parcel-bundler/parcel-benchmark-action#93 (comment)

So nevermind, feel free to merge this if all looks good.

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.

3 participants