Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

perf: optimize account init logging #4318

Merged
merged 3 commits into from
Jun 23, 2023
Merged

perf: optimize account init logging #4318

merged 3 commits into from
Jun 23, 2023

Conversation

davidmurdoch
Copy link
Member

No description provided.

@davidmurdoch davidmurdoch changed the title optimize account init logging perf: optimize account init logging May 5, 2023
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented May 5, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: e3476b4
Status: ✅  Deploy successful!
Preview URL: https://d62f6f24.ganache.pages.dev
Branch Preview URL: https://optimize-accounts.ganache.pages.dev

View logs

@davidmurdoch davidmurdoch marked this pull request as ready for review May 5, 2023 22:24
Copy link
Contributor

@jeffsmale90 jeffsmale90 left a comment

Choose a reason for hiding this comment

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

Maybe just over optimising here, but these look like some simple changes to bundle together.

const logs = [];
logs.push("");
logs.push("Available Accounts");
logs.push("==================");
if (addresses.length > 0) {
addresses.forEach(function (address, index) {
const balance = accounts[address].balance;
addresses.forEach(([address, account], index) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I checkout out this for benchmark: https://github.com/LeanyLabs/js-array-performance and reran with node 20.2.0 and found that forEach is even more slow compared with for of than it was in node 14.

Array.forEach x 130 ops/sec ±0.22% (84 runs sampled)
for of x 529 ops/sec ±0.57% (94 runs sampled)

While we are here, do you think it's worthwhile updating these Array.forEachs to for....ofs?

src/packages/cli/src/initialize/ethereum.ts Outdated Show resolved Hide resolved
@jeffsmale90
Copy link
Contributor

I made those additional optimizations in a separate review here: #4446 incase you don't like them.

@jeffsmale90 jeffsmale90 self-requested a review June 21, 2023 20:32
@davidmurdoch davidmurdoch merged commit fd5f05d into develop Jun 23, 2023
@davidmurdoch davidmurdoch deleted the optimize-accounts branch June 23, 2023 22:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants