zlib: implement fast path for crc32

[gurgunday]

Full benchmarks:

node % node benchmark/zlib/crc32.js
zlib/crc32.js n=4000000 len=32 type="buffer": 37,396,444.66876052
zlib/crc32.js n=4000000 len=256 type="buffer": 34,468,122.98396251
zlib/crc32.js n=4000000 len=4096 type="buffer": 4,642,218.20245069
zlib/crc32.js n=4000000 len=65536 type="buffer": 414,698.94918172585
zlib/crc32.js n=4000000 len=32 type="string": 27,544,938.95769763
zlib/crc32.js n=4000000 len=256 type="string": 13,360,556.974013995
zlib/crc32.js n=4000000 len=4096 type="string": 3,598,194.020364296
zlib/crc32.js n=4000000 len=65536 type="string": 247,640.72679581624
node % ./node benchmark/zlib/crc32.js
zlib/crc32.js n=4000000 len=32 type="buffer": 47,694,181.36925779
zlib/crc32.js n=4000000 len=256 type="buffer": 41,584,645.71789544
zlib/crc32.js n=4000000 len=4096 type="buffer": 6,730,071.896634588
zlib/crc32.js n=4000000 len=65536 type="buffer": 418,870.8874470888
zlib/crc32.js n=4000000 len=32 type="string": 34,844,878.72618591
zlib/crc32.js n=4000000 len=256 type="string": 24,282,737.956485238
zlib/crc32.js n=4000000 len=4096 type="string": 2,102,476.0393039584
zlib/crc32.js n=4000000 len=65536 type="string": 139,447.8455126307

[nodejs-github-bot]

Review requested:

• @nodejs/performance

[targos]

The V8 fast API is not a way to write fast paths with fallbacks. It must be able to handle all cases.
One way to implement a fast API function is to share actual C++ implementation with the slow one.
You can see an example here:

node/src/node_buffer.cc

Lines 566 to 604 in 96a749b

	void CopyImpl(Local<Value> source_obj,
	Local<Value> target_obj,
	const uint32_t target_start,
	const uint32_t source_start,
	const uint32_t to_copy) {
	ArrayBufferViewContents<char> source(source_obj);
	SPREAD_BUFFER_ARG(target_obj, target);
	memmove(target_data + target_start, source.data() + source_start, to_copy);
	}
	// Assume caller has properly validated args.
	void SlowCopy(const FunctionCallbackInfo<Value>& args) {
	Local<Value> source_obj = args[0];
	Local<Value> target_obj = args[1];
	const uint32_t target_start = args[2].As<Uint32>()->Value();
	const uint32_t source_start = args[3].As<Uint32>()->Value();
	const uint32_t to_copy = args[4].As<Uint32>()->Value();
	CopyImpl(source_obj, target_obj, target_start, source_start, to_copy);
	args.GetReturnValue().Set(to_copy);
	}
	// Assume caller has properly validated args.
	uint32_t FastCopy(Local<Value> receiver,
	Local<Value> source_obj,
	Local<Value> target_obj,
	uint32_t target_start,
	uint32_t source_start,
	uint32_t to_copy,
	// NOLINTNEXTLINE(runtime/references)
	FastApiCallbackOptions& options) {
	HandleScope scope(options.isolate);
	CopyImpl(source_obj, target_obj, target_start, source_start, to_copy);
	return to_copy;
	}

[gurgunday]

I see, thanks for showing an example

I'll take a look today

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.25%. Comparing base (b9fa656) to head (978501f).
⚠️ Report is 235 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #59813      +/-   ##
==========================================
- Coverage   89.95%   88.25%   -1.71%     
==========================================
  Files         667      701      +34     
  Lines      197207   206780    +9573     
  Branches    38523    39774    +1251     
==========================================
+ Hits       177407   182500    +5093     
- Misses      12242    16292    +4050     
- Partials     7558     7988     +430     

Files with missing lines	Coverage Δ
src/node_zlib.cc	78.11% <100.00%> (+0.12%)	⬆️

... and 116 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[gurgunday]

@targos I'll change the commit message as well to use Fast API instead of Fast Path

[mcollina]

rlgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/69162/

[targos]

LGTM with a CI run of the new benchmark.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/69178/

[targos]

Benchmark CI (not sure I put the right parameters): https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1733/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/69183/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/69185/

[targos]

++ cat output110925-150644.csv
++ Rscript benchmark/compare.R
                                                confidence improvement accuracy (*)   (**)  (***)
zlib/crc32.js n=4000000 len=256 type='buffer'          ***      0.97 %       ±0.05% ±0.07% ±0.09%
zlib/crc32.js n=4000000 len=256 type='string'          ***      6.91 %       ±0.47% ±0.63% ±0.82%
zlib/crc32.js n=4000000 len=32 type='buffer'           ***     46.82 %       ±0.13% ±0.17% ±0.22%
zlib/crc32.js n=4000000 len=32 type='string'           ***      1.86 %       ±0.17% ±0.23% ±0.30%
zlib/crc32.js n=4000000 len=4096 type='buffer'         ***      0.90 %       ±0.01% ±0.01% ±0.01%
zlib/crc32.js n=4000000 len=4096 type='string'                  0.05 %       ±0.16% ±0.21% ±0.27%
zlib/crc32.js n=4000000 len=65536 type='buffer'        ***      0.14 %       ±0.03% ±0.05% ±0.06%
zlib/crc32.js n=4000000 len=65536 type='string'                 0.12 %       ±0.18% ±0.24% ±0.32%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 8 comparisons, you can thus
expect the following amount of false-positive results:
  0.40 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.08 false positives, when considering a   1% risk acceptance (**, ***),
  0.01 false positives, when considering a 0.1% risk acceptance (***)

[gurgunday]

Can we rerun the CI? Not sure if Triagers can do it

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/69217/

[nodejs-github-bot]

Landed in c85460b

[aduh95]

This does not land cleanly on v22.x-staging and would require a manual backport PR if we want it on v22.x.

[gurgunday]

I can take a look at this