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

[FEATURE] minify task: Use workerpool for terser #875

Merged
merged 4 commits into from
Feb 2, 2023

Conversation

RandomByte
Copy link
Member

@RandomByte RandomByte commented Jan 21, 2023

Based on #700:

  • Changed logging to verbose
  • Added option 'useWorkers' to processor. Implicit activation through
    taskUtil parameter seemed a bit odd. Moved that to the minify task
  • Added JSDoc

@RandomByte RandomByte force-pushed the minify-task-workerpool branch 2 times, most recently from 54353f2 to ca3190e Compare January 21, 2023 11:19
@coveralls
Copy link

coveralls commented Jan 21, 2023

Coverage Status

Coverage: 94.623% (+0.01%) from 94.609% when pulling 28efc4c on minify-task-workerpool into 34b1e6f on main.

@RandomByte RandomByte force-pushed the minify-task-workerpool branch 2 times, most recently from 3ffea21 to fd5c77f Compare January 23, 2023 20:41
@RandomByte RandomByte marked this pull request as ready for review January 24, 2023 09:05
@RandomByte RandomByte force-pushed the minify-task-workerpool branch from fd5c77f to ea9df9b Compare January 25, 2023 16:54
Based on #700

* Changed logging to verbose
* Added option 'useWorkers' to processor. Implicit activation through
  taskUtil parameter seemed a bit odd. Moved that to the minify task
* Added JSDoc
@RandomByte RandomByte force-pushed the minify-task-workerpool branch from ea9df9b to 75463c3 Compare January 25, 2023 16:57
@RandomByte
Copy link
Member Author

RandomByte commented Jan 25, 2023

I did some benchmarks according to our documentation, comparing this PR to the current main branch:

openui5-sample-app

ui5-builder Ref Command Mean [ms] Min [ms] Max [ms] Relative
main (9aa8b1c) ui5 build 748.5 ± 20.9 725.2 803.0 Baseline
minify-task-workerpool (75463c3) ui5 build 796.5 ± 33.7 739.3 870.6 +7%

sap.ui.core

ui5-builder Ref Command Mean [s] Min [s] Max [s] Relative
main (9aa8b1c) ui5 build 10.915 ± 0.219 10.718 11.451 Baseline
minify-task-workerpool (75463c3) ui5 build 7.790 ± 0.061 7.659 7.856 -29%

OpenUI5 testsuite (including dependencies)

ui5-builder Ref Command Mean [s] Min [s] Max [s] Relative
main (9aa8b1c) ui5 build --all 86.258 ± 1.258 84.060 87.747 Baseline
minify-task-workerpool (75463c3) ui5 build --all 73.263 ± 1.187 70.492 74.629 -15%

Specs: MacBook Pro M1 Max

@RandomByte RandomByte requested a review from a team February 2, 2023 08:51
codeworrior
codeworrior previously approved these changes Feb 2, 2023
Copy link
Member

@codeworrior codeworrior left a comment

Choose a reason for hiding this comment

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

LGTM.

If myFun() in the test fixtures was meant to be myFunc(), you might want to fix that. But the tests still make sense in their current state. A reader just may wonder about the name mismatch.

@RandomByte RandomByte force-pushed the minify-task-workerpool branch from ea298bb to 28efc4c Compare February 2, 2023 12:18
Copy link
Member

@codeworrior codeworrior left a comment

Choose a reason for hiding this comment

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

LGTM

@RandomByte RandomByte merged commit ff4c6fb into main Feb 2, 2023
@RandomByte RandomByte deleted the minify-task-workerpool branch February 2, 2023 12:34
@RandomByte RandomByte mentioned this pull request Feb 7, 2023
3 tasks
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.

4 participants