test: add perf metrics utilities for write 1 / read 3 #1855

ddelgrosso1 · 2022-04-08T14:12:23Z

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

ddelgrosso1 · 2022-04-08T14:27:04Z

This is dependent on worker_threads which is not available until we drop node 10 support. Marking it as do not merge in the meantime.

performance-metrics/performPerformanceTest.ts

frankyn · 2022-04-08T16:42:52Z

internal-tooling/performPerformanceTest.ts

+
+const randomInteger = (minInclusive: number, maxInclusive: number) => {
+  return (
+    Math.floor(Math.random() * (maxInclusive - minInclusive + 1)) + minInclusive


add a comment or rename randomInteger to specify Uniform Distribution is expected

frankyn · 2022-04-12T22:22:31Z

internal-tooling/performPerformanceTest.ts

+  const sizeInBytes = generateRandomFile(fileName);
+
+  const stg = new Storage({
+    projectId: 'ddelgrosso-test',


You don't need to supply this value if it comes from ServiceAccount.json file. I would remove it.

Removed and made configurable from command line.

frankyn · 2022-04-12T22:26:08Z

internal-tooling/performPerformanceTest.ts

+      appBufferSize: BLOCK_SIZE_IN_BYTES,
+      libBufferSize: 16384, //Node default
+      crc32Enabled: false,
+      md5Enabled: false,


Why isn't validation not used for uploads? I thought it was enabled by default for uploads. Python and Go validate checksums on upload as well.

Made it uses same as download now.

frankyn · 2022-04-12T22:29:13Z

internal-tooling/performPerformanceTest.ts

@@ -0,0 +1,157 @@
+/*!


Recommend creating a README.md similar to Python to clarify this is for dev team usage:

Python: https://github.com/googleapis/python-storage/blob/9dea065241f893e7d5eef22d70671ef0f860bb23/tests/perf/README.md

frankyn · 2022-04-12T22:31:06Z

internal-tooling/performPerformanceTest.ts

+    } else if (checkType === 1) {
+      iterationResult.crc32Enabled = true;
+      start = performance.now();
+      await file.download({validation: 'crc32c'});


Does crc32c validation in this case expect that nodejs-storage is using the fast-crc32c package?

We don't expect anything. Validation is currently being offloaded to another library. If fast-crc32c is available it will use it, if not it uses its own logic. See here: https://github.com/stephenplusplus/hash-stream-validation/blob/0aa27468552a2b12cff1813b5ef63ecb4e903621/index.js#L5

frankyn · 2022-04-12T22:31:48Z

internal-tooling/performPerformanceTest.ts

+    const checkType = randomInteger(0, 2);
+    if (checkType === 0) {
+      start = performance.now();
+      await bucket.file(`${fileName}`).download({validation: false});


Per meeting: download to file

frankyn · 2022-04-12T22:42:02Z

internal-tooling/performPerformanceTest.ts

+      op: 'WRITE',
+      objectSize: sizeInBytes,
+      appBufferSize: BLOCK_SIZE_IN_BYTES,
+      libBufferSize: 16384, //Node default


assign 16384 to a constant variable and link to the where this is defined in the Node.js library for future us.

frankyn · 2022-04-14T05:01:17Z

internal-tooling/performPerformanceTest.ts

+
+    bucket = stg.bucket(argv.bucket, {
+      preconditionOpts: {
+        ifGenerationMatch: 0,


I believe this should go on bucket.upload() call no?

When we implemented preconditions due to constraints with common (i.e. we couldn't pass preconditions per function), we had to pass them at the constructor level. One of the upcoming refactors with internalizing common is adding the ability to pass these per function. If you look at the code below these lines you will see I re-declare the bucket as necessary to remove the preconditions where appropriate.

I literally just spoke with @shaffeeullah and she clarified this as well (in-person no way!!); thanks for clarifying, it will get better just didn't realize this was a trade off with not internalizing common. It's okay, not great but at least it's possible to pass in the precondition.

shaffeeullah

I reviewed mostly for code clarity. Will lean on @frankyn to take a closer look at the functionality.

shaffeeullah · 2022-05-02T15:13:22Z

internal-tooling/README.md

@@ -0,0 +1,44 @@
+# nodejs-storage benchmarking
+
+**This is not an officially supported Google product**


Change This is not an officially supported Google product to This is not a supported Google product. It's not unofficially supported either.

shaffeeullah · 2022-05-02T15:14:01Z

internal-tooling/README.md

+
+**This is not an officially supported Google product**
+
+This benchmarking script is used by Storage client library maintainers to benchmark various workloads and collect metrics in order to improve performance of the library.


"This benchmarking script is used by Storage client" -> "This benchmarking script intended for use by Storage client"

shaffeeullah · 2022-05-02T15:15:35Z

internal-tooling/performPerformanceTest.ts

+  parentPort?.postMessage(results);
+}
+
+async function performWriteReadTest(): Promise<TestResult[]> {


Optional: add a small docstring for each function

ddelgrosso1 · 2022-05-02T15:28:09Z

I've repointed this at the dropNode10 branch since it utilizes Node 12 specific features. I still had to disable the warning regarding worker_threads as this was not supported in the initial release of Node 12. However, since we added a warning that this is unsupported I feel this is an acceptable compromise to get this merged.

ddelgrosso1 · 2022-05-09T20:24:58Z

@frankyn any further comments? I would like to get this merged for release with the drop in support for node 10.

* test: add performance measuring * add threading support to performance tests * linter fixes * rename folder * add precondition to uploads, write to file for downloads * fix pathing * review feedback * linter * disable no unsupported features error now node 10 is unsupported * add jsdoc headers

ddelgrosso1 requested review from a team as code owners April 8, 2022 14:12

product-auto-label bot added size: m Pull request size is medium. api: storage Issues related to the googleapis/nodejs-storage API. labels Apr 8, 2022

ddelgrosso1 added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 8, 2022

shaffeeullah reviewed Apr 8, 2022

View reviewed changes

performance-metrics/performPerformanceTest.ts Outdated Show resolved Hide resolved

product-auto-label bot added size: xs Pull request size is extra small. and removed size: m Pull request size is medium. labels Apr 8, 2022

ddelgrosso1 force-pushed the perf-metrics branch from 96380eb to b1ad619 Compare April 8, 2022 14:47

product-auto-label bot added size: m Pull request size is medium. and removed size: xs Pull request size is extra small. labels Apr 8, 2022

ddelgrosso1 requested a review from frankyn April 12, 2022 21:18

frankyn requested changes Apr 12, 2022

View reviewed changes

ddelgrosso1 force-pushed the perf-metrics branch from b1ad619 to 22c14b7 Compare April 13, 2022 14:50

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Apr 13, 2022

frankyn requested changes Apr 14, 2022

View reviewed changes

ddelgrosso1 changed the base branch from main to dropNode10 April 28, 2022 16:09

ddelgrosso1 added do not merge Indicates a pull request not ready for merge, due to either quality or timing. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Apr 28, 2022

ddelgrosso1 added 9 commits May 2, 2022 14:39

test: add performance measuring

fe51d55

add threading support to performance tests

790a194

linter fixes

212a5e5

rename folder

4265fa4

add precondition to uploads, write to file for downloads

eaa3764

fix pathing

6b970df

review feedback

90eafe7

linter

3c7f68d

disable no unsupported features error now node 10 is unsupported

7bd3b03

ddelgrosso1 force-pushed the perf-metrics branch from 08470c9 to 7bd3b03 Compare May 2, 2022 14:43

ddelgrosso1 removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 2, 2022

ddelgrosso1 requested review from frankyn and shaffeeullah May 2, 2022 14:46

shaffeeullah reviewed May 2, 2022

View reviewed changes

add jsdoc headers

d4cad55

frankyn approved these changes May 9, 2022

View reviewed changes

ddelgrosso1 merged commit ec56333 into googleapis:dropNode10 May 10, 2022

ddelgrosso1 deleted the perf-metrics branch May 10, 2022 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add perf metrics utilities for write 1 / read 3 #1855

test: add perf metrics utilities for write 1 / read 3 #1855

ddelgrosso1 commented Apr 8, 2022

ddelgrosso1 commented Apr 8, 2022

frankyn Apr 8, 2022

ddelgrosso1 Apr 13, 2022

frankyn Apr 12, 2022

ddelgrosso1 Apr 13, 2022

frankyn Apr 12, 2022

ddelgrosso1 Apr 13, 2022

frankyn Apr 12, 2022

ddelgrosso1 Apr 13, 2022

frankyn Apr 12, 2022

ddelgrosso1 Apr 13, 2022

frankyn Apr 12, 2022

ddelgrosso1 Apr 13, 2022

frankyn Apr 12, 2022

ddelgrosso1 Apr 13, 2022

frankyn Apr 14, 2022

ddelgrosso1 Apr 14, 2022

frankyn Apr 14, 2022

shaffeeullah left a comment

shaffeeullah May 2, 2022

shaffeeullah May 2, 2022

shaffeeullah May 2, 2022

ddelgrosso1 commented May 2, 2022

ddelgrosso1 commented May 9, 2022

		@@ -0,0 +1,44 @@
		# nodejs-storage benchmarking

		This is not an officially supported Google product


		This is not an officially supported Google product

		This benchmarking script is used by Storage client library maintainers to benchmark various workloads and collect metrics in order to improve performance of the library.

test: add perf metrics utilities for write 1 / read 3 #1855

test: add perf metrics utilities for write 1 / read 3 #1855

Conversation

ddelgrosso1 commented Apr 8, 2022

ddelgrosso1 commented Apr 8, 2022

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

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

shaffeeullah 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

ddelgrosso1 commented May 2, 2022

ddelgrosso1 commented May 9, 2022