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

New StylesheetProcessor implementation is extremly slow #1872

Closed
skrtheboss opened this issue Feb 11, 2021 · 12 comments
Closed

New StylesheetProcessor implementation is extremly slow #1872

skrtheboss opened this issue Feb 11, 2021 · 12 comments
Assignees

Comments

@skrtheboss
Copy link
Contributor

Type of Issue

The new implementation of the StylesheetProcessor using workers is extremly slow

[x] Bug Report
[ ] Feature Request

Description

I have added a console.time('StylesheetProcessor') at the beginning of the StylesheetProcessor.process call and a console.timeEnd('StylesheetProcessor') before the css return statement, these are the results:

ng-packagr@11.1.4

- Compiling TypeScript sources through NGC
StylesheetProcessor: 12.902ms
StylesheetProcessor: 8.599ms
StylesheetProcessor: 8.728ms
StylesheetProcessor: 19.27ms
StylesheetProcessor: 13.624ms
StylesheetProcessor: 9.034ms
StylesheetProcessor: 10.043ms
StylesheetProcessor: 24.805ms
StylesheetProcessor: 28.633ms
StylesheetProcessor: 7.321ms
StylesheetProcessor: 44.461ms
StylesheetProcessor: 21.771ms
StylesheetProcessor: 19.549ms
StylesheetProcessor: 6.383ms
StylesheetProcessor: 13.473ms
StylesheetProcessor: 6.555ms
StylesheetProcessor: 21.906ms
StylesheetProcessor: 8.931ms
StylesheetProcessor: 8.84ms
StylesheetProcessor: 18.113ms

ng-packagr@11.2.0

- Compiling TypeScript sources through NGC
StylesheetProcessor: 1.396s
StylesheetProcessor: 1.438s
StylesheetProcessor: 1.391s
StylesheetProcessor: 1.408s
StylesheetProcessor: 1.354s
StylesheetProcessor: 1.374s
StylesheetProcessor: 1.383s
StylesheetProcessor: 1.435s
StylesheetProcessor: 1.945s
StylesheetProcessor: 2.004s
StylesheetProcessor: 1.703s
StylesheetProcessor: 1.619s
StylesheetProcessor: 1.657s
StylesheetProcessor: 1.484s
StylesheetProcessor: 1.579s
StylesheetProcessor: 1.456s
StylesheetProcessor: 1.438s
StylesheetProcessor: 1.362s
StylesheetProcessor: 1.368s
StylesheetProcessor: 1.428s

I think the problem is, that every time a file must be processed a worker is started, instead of reusing an existing one.

Version Information

@angular/*: 11.2.0
typescript: 4.1.5
rxjs: 6.3.3
node: 14.15.4
npm/yarn:  1.22.5
@alan-agius4
Copy link
Member

alan-agius4 commented Feb 11, 2021

Hi @skrtheboss,

Are you using Windows by any chance?

Re-using the worker would definitely be ideal, but in this case it is not possible due to the sync nature of the operations being done.

The worker is used to transformer async operations to sync operations.

@flash-me
Copy link

Hi @skrtheboss,

Are you using Windows by any chance?

Re-using the worker would definitely be ideal, but in this case it is not possible due to the sync nature of the operations being done.

The worker is used to transformer async operations to sync operations.

@alan-agius4
Holy... like if the I/O and filesystem perf on windows isn't already shitty enough, now for each style file a complete new node process is created. *nix systems may just fork the parent with few to none perf impact, but on sh!tty Windows this means CreateProcess and all the f@cking overhead it comes with, for each style file! 🥺

Anyway, the reason for this is just to synchronously call async code? Seriously? Using child_process / execfileSync is really fancy, tbh 😅, but comes also with huge drawbacks, esp on winsh!t OS.
Couldn't this solved by making use of generators?
Here a brief article with examples . Actually hard to find, since usually people tend to not block execution..

(AFAIK sass runs way faster when called synchronous, but I see the intent. handling all preprocessors different would blow up the code...)

cheers
flash ⚡

@alan-agius4
Copy link
Member

Actually the reason behind this is that both LESS and PostCSS are Async, however the TypeScript compiler is Sync.

@flash-me, thanks for the above article, but unless I am missing something that example shared is still asynchronous.

To be more precise the line below;

run(function* myDelayedMessages(resume) {
    console.log(yield delay(1000, resume));
    console.log(yield delay(1200, resume));
})

@skrtheboss
Copy link
Contributor Author

Hi @alan-agius4,

Yes I am in deed using windows... But i found a better solution for solving this problem and i have created a pull request.
Its inspired by how its done in the babel-plugin-postcss usin the sync-rpc library.

On my machine the performance is way better now:

- Compiling TypeScript sources through NGC
StylesheetProcessor: 113.31ms
StylesheetProcessor: 114.149ms
StylesheetProcessor: 113.765ms
StylesheetProcessor: 116.981ms
StylesheetProcessor: 125.486ms
StylesheetProcessor: 112.848ms
StylesheetProcessor: 107.247ms
StylesheetProcessor: 121.265ms
StylesheetProcessor: 119.401ms
StylesheetProcessor: 110.727ms
StylesheetProcessor: 222.209ms
StylesheetProcessor: 109.632ms
StylesheetProcessor: 115.947ms
StylesheetProcessor: 108.475ms
StylesheetProcessor: 112.964ms
StylesheetProcessor: 112.828ms
StylesheetProcessor: 119.923ms
StylesheetProcessor: 98.234ms
StylesheetProcessor: 105.52ms
StylesheetProcessor: 115.459ms

@alan-agius4
Copy link
Member

@skrtheboss, oh wow that's an interesting package. Thanks for sharing let me try it out.

alan-agius4 pushed a commit that referenced this issue Feb 12, 2021
…n to sync functions #1872

By using sync-rpc ( which is also used by the babel-plugin-postcss package ), we increase the performance
over execFileSync calls, which are very expensive.
@alan-agius4
Copy link
Member

Closed via #1875

alan-agius4 pushed a commit that referenced this issue Feb 12, 2021
…n to sync functions #1872

By using sync-rpc ( which is also used by the babel-plugin-postcss package ), we increase the performance
over execFileSync calls, which are very expensive.

(cherry picked from commit 9ccafb0)
@flash-me
Copy link

Actually the reason behind this is that both LESS and PostCSS are Async, however the TypeScript compiler is Sync.

@flash-me, thanks for the above article, but unless I am missing something that example shared is still asynchronous.

To be more precise the line below;

run(function* myDelayedMessages(resume) {
    console.log(yield delay(1000, resume));
    console.log(yield delay(1200, resume));
})

Sure @alan-agius4, here is an abstract example:

function shortAsync() {
  return new Promise(resolve =>
    setTimeout(() => {
      console.log("Short Async");
      resolve();
    }, 1000)
  );
}

function longAsync() {
  return new Promise(resolve =>
    setTimeout(() => {
      console.log("Long Async");
      resolve();
    }, 5000)
  );
}

/**
 * This is completely synchronous
 */
const syncRoutine = (function*() {
  // Wait 3 seconds, even though its async
  yield longAsync().then(() => syncRoutine.next());

  // Usually this would be handled faster
  yield shortAsync().then(() => syncRoutine.next());
})();

syncRoutine.next();

You can return information with yield, but also pass args to the generator function with next

cheers
flash ⚡

@alan-agius4
Copy link
Member

@flash-me, unless I am missing something (which I might cause I am checking from my mob) the above is still async and non blocking, it’s faking sync similar to async/await.

This can be tested by doing the below;

console.log('before')
syncRoutine.next();
console.log('after')
> "before"
> "after"
> "Long Async"
> "Short Async

@flash-me
Copy link

flash-me commented Feb 12, 2021

This can be tested by doing the below;

console.log('before')
syncRoutine.next();
console.log('after')

That's because you don't have an yield on the first / "outer" call.
This would be the piece of code, where you could have async behaviour and don't rely on synchronous processing.

e.g. here, which is an async context, you would call the first next and pass all the paths to it and the stylepreprocessor itself would run synchronously inside, file by file, by making use of yield

cheers
flash ⚡

@alan-agius4 alan-agius4 self-assigned this Feb 12, 2021
@alan-agius4
Copy link
Member

We don’t know all the file paths beforehand, we only know one file at a time.

readResource: (fileName: string) => {

@alan-agius4
Copy link
Member

In version 12, we did some changes which should improve the speed even more. This is because in v12 we no longer support Node.JS 10 and thus we can use receiveMessageOnPort workers API together with Atomics.

#1878

@github-actions
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

This action has been performed automatically by a bot.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

3 participants