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

infra(locales): use mostly async io for generate-locales #2826

Merged
merged 6 commits into from
Apr 17, 2024

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Apr 15, 2024

Fixes #2804

Execution Stats limited all
next 12s 34s
async-io 10s 25s
async-io v2 9s 11s

time pnpm run generate:locales

Please note that the main perfomance boost in this PR is caused by the Promise.all cases.
There might be better ways to async everything.

We could also safe up to 2s by removing the Running data normalization logs.

@ST-DDT ST-DDT added p: 1-normal Nothing urgent c: locale Permutes locale definitions c: infra Changes to our infrastructure or project setup labels Apr 15, 2024
@ST-DDT ST-DDT added this to the vAnytime milestone Apr 15, 2024
@ST-DDT ST-DDT self-assigned this Apr 15, 2024
@ST-DDT ST-DDT requested a review from a team as a code owner April 15, 2024 13:46
Copy link

netlify bot commented Apr 15, 2024

Deploy Preview for fakerjs ready!

Name Link
🔨 Latest commit c43ff8d
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/661fe71909e09900082536d1
😎 Deploy Preview https://deploy-preview-2826.fakerjs.dev
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@ST-DDT ST-DDT requested a review from a team April 15, 2024 13:47
@ST-DDT
Copy link
Member Author

ST-DDT commented Apr 15, 2024

Please also post your performance statistics for comparison.

Copy link

codecov bot commented Apr 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.96%. Comparing base (462c80e) to head (c43ff8d).

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #2826   +/-   ##
=======================================
  Coverage   99.95%   99.96%           
=======================================
  Files        2964     2964           
  Lines      212544   212544           
  Branches      948      951    +3     
=======================================
+ Hits       212442   212461   +19     
+ Misses        102       83   -19     

see 2 files with indirect coverage changes

@Shinigami92
Copy link
Member

next 6.44s user 0.51s system 149% cpu 4.639 total
this PR 6.41s user 0.64s system 190% cpu 3.696 total

so in total it safes a second, cause it utilizes the CPU better 👍

@ST-DDT
Copy link
Member Author

ST-DDT commented Apr 15, 2024

And if you run it for all files?

@Shinigami92
Copy link
Member

And if you run it for all files?

next 17.03s user 2.44s system 139% cpu 13.914 total
this PR 12.73s user 1.70s system 181% cpu 7.961 total

@matthewmayer
Copy link
Contributor

matthewmayer commented Apr 15, 2024

can we skip the de-duping given that we already have a test checking for duplicates at test/locale-data.spec.ts

@xDivisionByZerox
Copy link
Member

can we skip the de-duping given that we already have a test checking for duplicates at test/locale-data.spec.ts

Not so sure about this right now. Maybe remove this after all modules are active? Currently, it makes creating PRs really easy, as the only thing you have to do is:

  1. Remove the module to normalize
  2. Run locale generation

I get that you could "easily" achieve this with a separate script, but this way, every person could create a PR right now.

@matthewmayer
Copy link
Contributor

can we skip the de-duping given that we already have a test checking for duplicates at test/locale-data.spec.ts

Not so sure about this right now. Maybe remove this after all modules are active? Currently, it makes creating PRs really easy, as the only thing you have to do is:

  1. Remove the module to normalize
  2. Run locale generation

I get that you could "easily" achieve this with a separate script, but this way, every person could create a PR right now.

but i mean, we already know there are no existing duplicates, because if there were duplicates, the test would fail?

@matthewmayer
Copy link
Contributor

can we skip the de-duping given that we already have a test checking for duplicates at test/locale-data.spec.ts

Not so sure about this right now. Maybe remove this after all modules are active? Currently, it makes creating PRs really easy, as the only thing you have to do is:

  1. Remove the module to normalize
  2. Run locale generation

I get that you could "easily" achieve this with a separate script, but this way, every person could create a PR right now.

but i mean, we already know there are no existing duplicates, because if there were duplicates, the test would fail?

removing the "new Set" for deduping doesn't seem to speed things up significantly.

@matthewmayer
Copy link
Contributor

matthewmayer commented Apr 16, 2024

Seems like you can save a significant amount of time (~10 seconds) by returning early if the data is unchanged. This saves pretty-printing and writing out the data if there is no effect:

const localeData = normalizeDataRecursive(fileImport.default);
const original = JSON.stringify(fileImport.default);
const modified = JSON.stringify(localeData);
if (original === modified) {
    return;
}

@xDivisionByZerox
Copy link
Member

but i mean, we already know there are no existing duplicates, because if there were duplicates, the test would fail?

You are totally right. I was thinking about something different. May bad...

@ST-DDT
Copy link
Member Author

ST-DDT commented Apr 16, 2024

Seems like you can save a significant amount of time (~10 seconds) by returning early if the data is unchanged.

Awesome idea. Here the new results for my machine:

Execution Stats limited all
next 12s 34s
async-io 10s 25s
async-io v2 9s 11s

Please confirm the time on your devices as well.

@ST-DDT ST-DDT requested review from Shinigami92 and a team April 16, 2024 11:28
@Shinigami92
Copy link
Member

Shinigami92 commented Apr 16, 2024

Please confirm the time on your devices as well.

version user user improvement system cpu total total improvement
limited base 6.44s - 0.51s 149% 4.639 -
limited v1 6.41s 0.03s 0.64s 190% 3.696 0.943
limited v2 6.09s 0.35s 0.63s 174% 3.838 0.801
all base 17.03s - 2.44s 139% 13.914 -
all v1 12.73s 4.30s 1.70s 181% 7.961 5.953
all v2 10.94s 6.09s 1.52s 187% 6.656 7.258

tested on Mac Pro M1

@ST-DDT ST-DDT requested review from a team April 16, 2024 19:41
@ST-DDT ST-DDT enabled auto-merge (squash) April 17, 2024 15:13
@ST-DDT ST-DDT merged commit e1d8e4d into next Apr 17, 2024
20 checks passed
@ST-DDT ST-DDT deleted the scripts/generate-locales/async-io branch April 17, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: infra Changes to our infrastructure or project setup c: locale Permutes locale definitions p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Speed up normalisation code
4 participants