[DRAFT] - [perf] - wasm optimisations #6

pivanov · 2024-09-03T09:22:11Z

Pre-flight checklist

I have read the Contributing Guidelines on pull requests.
Commit messages follow the conventional commits spec
If this is a code change: I have written unit tests.
If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

Test Plan

Related issues/PRs

…late-file-size scripts

…son file

valeriivanchev

Great work. Just a couple of comments and also two points to research.

Check the difference between || and ?? and see if we have used it appropriately.
Check how pnpm --filter works.( e.g. sub-packages installation, context in which the scripts are running).
Check the formatting in the code.

valeriivanchev · 2024-09-11T09:01:44Z

benchmark/__tests__/results-store.js

+    const tempResults = {};
+    tempResults[testName] = result;
+
+    saveResults(tempResults);


Do we use this function anywhere else seems like it is only used in this one place. Maybe we don't need it?

Yes, it's true, I will remove it and move the code to addTestResults. Thanks

valeriivanchev · 2024-09-11T09:03:52Z

benchmark/__tests__/util.js

+export function logTestResults(testResults) {
+  // Determine which function to use based on the structure of the results
+  const isMinimal = testResults.result.receipts_outcome.length === 2;
+  const generateGasObjectFn = isMinimal


Imho: We can make this as a one function.

valeriivanchev · 2024-09-11T09:05:38Z

benchmark/__tests__/util.js

+        details.gasUsedToRefundUnusedGas,
+        jsEntry ? jsEntry[1].gasUsedToRefundUnusedGas : "",
+      ]);
+      rows.push([


No need to make a push to the same array in two places you can do it in one go. You can take a look from here

valeriivanchev · 2024-09-11T09:07:22Z

packages/shared-scripts/src/calculate-file-sizes.js

+
+    // Check if the JSON file already exists and load data
+    if (
+      await fs


Maybe we could refactor that part. Just to be more readable

Yea, good point i will change it.

valeriivanchev · 2024-09-11T09:15:57Z

benchmark/__tests__/util.js

+
+  const sortedData = {};
+
+  order.forEach((key) => {


Why not use array filtration?

valeriivanchev · 2024-09-11T09:18:41Z

packages/shared-scripts/src/calculate-file-sizes.js

+    // Filter .wasm files and calculate sizes
+    const wasmFiles = files.filter((file) => path.extname(file) === ".wasm");
+
+    const fileSizePromises = wasmFiles.map(async (file) => {


Maybe we can refactor that part.

dim-daskalov · 2024-09-11T09:27:27Z

Great work. Just a couple of comments and also two points to research.

Check the difference between || and ?? and see if we have used it appropriately.

Check how pnpm --filter works.( e.g. sub-packages installation, context in which the scripts are running).

Check the formatting in the code.

Thanks for the review. I will check this out.

…, format the code and implement the comment suggestions

…dded

pivanov

Sorry for all of the comments :)
hope they will help!

🙇 Good Job!!!

pivanov · 2024-09-17T09:52:26Z

benchmark/__tests__/results-store.js

+  return {};
+}
+
+/** Function to add test results to the report if the GENERATE_REPORT environment variable is set to "true" */


Better Type Annotation (for TypeScript or JSDoc):

/** * @param {string} testName - The name of the test. * @param {Object} result - The test result object. */ export function addTestResults(testName, result) { ...

pivanov · 2024-09-17T09:53:29Z

benchmark/__tests__/results-store.js

Lack of error handling:
If there is an error while reading or writing the file, it can cause the program to crash without any clear indication. You should wrap file operations in try-catch blocks to gracefully handle potential file system errors.

Yea, you're right. Thanks for pointing that out.

pivanov · 2024-09-17T09:54:03Z

benchmark/__tests__/results-store.js

+function readExistingResults() {
+  if (fs.existsSync(resultsFilePath)) {
+    const fileContent = fs.readFileSync(resultsFilePath, "utf-8");
+    return JSON.parse(fileContent);


The JSON.parse() call could throw an error if the file contents are malformed or not valid JSON. It's good to wrap this in a try-catch block to handle such cases.

pivanov · 2024-09-17T09:56:32Z

benchmark/__tests__/results-store.js

+    };
+
+    // Write the combined results to the file
+    fs.writeFileSync(resultsFilePath, JSON.stringify(combinedResults, null, 2));


pivanov · 2024-09-17T09:57:23Z

benchmark/.gitignore

+build
+file-sizes.json


add new line here ;)
POSIX ... If a file without a trailing newline is modified, Git's diff algorithm might show unexpected differences because the change will also affect the last line. Adding a newline at the end helps prevent diffs from being misleading.

Nice, I did not know that. Thanks for mentioning it.

pivanov · 2024-09-17T10:15:56Z

packages/shared-scripts/src/calculate-file-sizes.js

+      Object.keys(updatedFileSizes.afterOptimization).length
+    ) {
+      // Generate Markdown file
+      generateMarkdown(scriptDir, fileSizes);


While you do catch errors in calculateFileSizes, some of the operations (e.g., generateMarkdown) don't have explicit error handling. It’s a good idea to wrap generateMarkdown in a try-catch block in case there are issues during markdown generation or file operations.

pivanov · 2024-09-17T10:16:59Z

packages/shared-scripts/src/calculate-file-sizes.js

+};
+
+// Function to calculate percentage difference
+const calculatePercentageDifference = (beforeSize, afterSize) => {


The calculatePercentageDifference function may throw an error if beforeSize or afterSize cannot be parsed properly (e.g., if there are unexpected non-numeric values). Adding error handling here will ensure the script doesn't crash unexpectedly.

const calculatePercentageDifference = (beforeSize, afterSize) => { const beforeSizeNum = parseFloat(beforeSize); const afterSizeNum = parseFloat(afterSize); if (isNaN(beforeSizeNum) || isNaN(afterSizeNum)) { return "N/A"; } return ( (((beforeSizeNum - afterSizeNum) / beforeSizeNum) * 100).toFixed(2) + "%" ); };

pivanov · 2024-09-17T10:20:03Z

packages/shared-scripts/src/calculate-file-sizes.js

Improving CLI UX:
You’re already checking for --before and --after, but adding a help flag (--help) or making the usage output more verbose would improve the user experience. You could also validate if both flags (--before and --after) are provided, and throw an error if both are used simultaneously, as it might be confusing.

if (isBefore && isAfter) { console.log("Please specify either --before or --after, not both."); process.exit(1); } if (args.includes("--help")) { console.log("Usage: node script.js <relativeDir> [--before|--after]"); process.exit(0); }

pivanov · 2024-09-17T10:21:30Z

tests/.gitignore

+build
+file-sizes.json


pivanov · 2024-09-17T10:21:47Z

tests/package.json

    "near-sdk-js": "workspace:*"
  }
-}
+}


aaaand new line :)

dim-daskalov · 2024-09-17T11:43:48Z

Sorry for all of the comments :) hope they will help!

🙇 Good Job!!!

Hey @pivanov, thanks for the suggestions!

I learned a few new things.

All should be addressed now.

…tGas methods, fix the incorrect versioning

[perf] - wasm optimisations

3047a36

pivanov changed the title ~~[perf] - wasm optimisations~~ [DRAFT] - [perf] - wasm optimisations Sep 3, 2024

dim-daskalov added 2 commits September 9, 2024 18:19

add the shared-scripts package, include the generate-report and calcu…

d87245f

…late-file-size scripts

add a README.md for the shared script and .gitignore the file-sizes.j…

10b3c7a

…son file

valeriivanchev requested changes Sep 11, 2024

View reviewed changes

address Valeri's comments, move the shared-scripts to devDependencies…

2274270

…, format the code and implement the comment suggestions

dim-daskalov requested a review from valeriivanchev September 11, 2024 12:45

dim-daskalov added 2 commits September 11, 2024 17:43

remove the redundant shared-scripts from package.json files

098bacf

optimize the redundant code and add docstrings on the new functions a…

32d3558

…dded

dim-daskalov requested a review from gtsonevv September 17, 2024 07:12

gtsonevv approved these changes Sep 17, 2024

View reviewed changes

pivanov commented Sep 17, 2024

View reviewed changes

address Pavkata's comments, added error handling for file operations

dab3d33

address Pavkata's comments, remove redundand code, optimize the forma…

de6034d

…tGas methods, fix the incorrect versioning

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DRAFT] - [perf] - wasm optimisations #6

[DRAFT] - [perf] - wasm optimisations #6

pivanov commented Sep 3, 2024

valeriivanchev left a comment

valeriivanchev Sep 11, 2024

dim-daskalov Sep 11, 2024

valeriivanchev Sep 11, 2024

valeriivanchev Sep 11, 2024

valeriivanchev Sep 11, 2024

dim-daskalov Sep 11, 2024

valeriivanchev Sep 11, 2024

valeriivanchev Sep 11, 2024

dim-daskalov commented Sep 11, 2024

pivanov left a comment

pivanov Sep 17, 2024

pivanov Sep 17, 2024

dim-daskalov Sep 17, 2024

pivanov Sep 17, 2024

pivanov Sep 17, 2024

pivanov Sep 17, 2024

dim-daskalov Sep 17, 2024

pivanov Sep 17, 2024

pivanov Sep 17, 2024

pivanov Sep 17, 2024

pivanov Sep 17, 2024

pivanov Sep 17, 2024

dim-daskalov commented Sep 17, 2024 •

edited

Loading

		build
		file-sizes.json

		build
		file-sizes.json

[DRAFT] - [perf] - wasm optimisations #6

Are you sure you want to change the base?

[DRAFT] - [perf] - wasm optimisations #6

Conversation

pivanov commented Sep 3, 2024

Pre-flight checklist

Motivation

Test Plan

Related issues/PRs

valeriivanchev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dim-daskalov commented Sep 11, 2024

pivanov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dim-daskalov commented Sep 17, 2024 • edited Loading

dim-daskalov commented Sep 17, 2024 •

edited

Loading