Bugfix and performance improvements by onigoetz · Pull Request #3 · lambci/sync-threads

onigoetz · 2023-12-22T12:57:21Z

I found a bug in the library and got a bit carried away when fixing it, this PR contains the following (each in their own commit, so can be reviewed separately if needed)

Tests for common features
A bugfix (moving the SharedArrayBuffer inside the function call, because if it was not done, the function would always return the result of the first call)
Support for multiple arguments in async functions (with a test)
New argument for timeouts (with a test)
Use a single worker for a given async function (big performance improvement, see below)
Add a benchmark

Reusing the worker across calls is a significant performance improvement as you can see:

Load time
┌─────────┬────────────────────────┬───────────┬────────────────────┬──────────┬─────────┐
│ (index) │       Task Name        │  ops/sec  │ Average Time (ns)  │  Margin  │ Samples │
├─────────┼────────────────────────┼───────────┼────────────────────┼──────────┼─────────┤
│    0    │        'native'        │ '818 234' │ 1222.1439571458939 │ '±3.69%' │  81824  │
│    1    │     'sync-threads'     │ '927 568' │ 1078.087884336715  │ '±0.94%' │  92757  │
│    2    │ 'sync-threads (1.0.1)' │ '902 893' │ 1107.5497360136544 │ '±2.00%' │  90290  │
│    3    │       'synckit'        │ '958 314' │ 1043.4987350162614 │ '±2.21%' │  95832  │
└─────────┴────────────────────────┴───────────┴────────────────────┴──────────┴─────────┘
Execution time
┌─────────┬────────────────────────┬───────────┬────────────────────┬───────────┬─────────┐
│ (index) │       Task Name        │  ops/sec  │ Average Time (ns)  │  Margin   │ Samples │
├─────────┼────────────────────────┼───────────┼────────────────────┼───────────┼─────────┤
│    0    │        'native'        │ '127 639' │ 7834.536944837339  │ '±3.20%'  │  12764  │
│    1    │     'sync-threads'     │ '10 695'  │  93495.3008379255  │ '±7.74%'  │  1071   │
│    2    │ 'sync-threads (1.0.1)' │   '881'   │ 1134772.9650776037 │ '±49.95%' │   89    │
│    3    │       'synckit'        │  '1 789'  │ 558809.5973989817  │ '±22.36%' │   179   │
└─────────┴────────────────────────┴───────────┴────────────────────┴───────────┴─────────┘
Total time
┌─────────┬────────────────────────┬──────────┬────────────────────┬───────────┬─────────┐
│ (index) │       Task Name        │ ops/sec  │ Average Time (ns)  │  Margin   │ Samples │
├─────────┼────────────────────────┼──────────┼────────────────────┼───────────┼─────────┤
│    0    │        'native'        │ '73 668' │ 13574.320538260483 │ '±10.58%' │  7367   │
│    1    │     'sync-threads'     │ '2 169'  │ 460920.5601951494  │ '±7.17%'  │   217   │
│    2    │ 'sync-threads (1.0.1)' │ '1 228'  │ 813877.0010413193  │ '±58.56%' │   123   │
│    3    │       'synckit'        │  '761'   │ 1313029.3481143904 │ '±41.65%' │   81    │
└─────────┴────────────────────────┴──────────┴────────────────────┴───────────┴─────────┘

These tests were run today on my own MacBook Air m2.
I did not push the test for sync-threads (1.0.1) as it's just to illustrate the point.

JounQin · 2023-12-26T02:44:43Z

@onigoetz Thanks for showing me the impressive performance improvement! Do you think is there anything we can change to bring better performance for synckit? It is slower in your benchmark because:

It does a lot of extra things than sync-threads, for example
1. worker path resolution to determine whether it should be considered as commonjs or ESM instead
2. first-class TypeScript support
3. first-class yarn P'n'P support
It does not use v8 for serialization/deserialization, maybe we can try this approach to improve synckit?

onigoetz · 2023-12-26T07:23:05Z

Hi, yes indeed it must be possible to bring these things to synckit as well.

The reason I am interested in bringing this PR to this package is the package size; sync-threads is 2-3kB, no dependencies. Whereas synckit is 1MB when installed with all its dependencies.

JounQin · 2023-12-26T08:14:14Z

The reason I am interested in bringing this PR to this package is the package size; sync-threads is 2-3kB, no dependencies. Whereas synckit is 1MB when installed with all its dependencies.

Indeed, that's because what I mentioned above

It does a lot of extra things than sync-threads

It's much more powerful. And well maintained.

Do you think what can I do for speeding up for synckit, or a PR would be very appreciated.

onigoetz · 2023-12-26T08:48:38Z

It's much more powerful. And well maintained.

Indeed, I see that it covers a lot of edge cases. Great work there :)

I'm up for the challenge and would happily make a PR to synckit.

Though I have to admit that if possible I would also like to make a PR to remove @pkgr/utils because it brings a lot of dependencies.

mhart · 2023-12-26T09:46:59Z

Awesome, thanks for this @onigoetz ! As you can tell, I like my libraries nice and lean, so I'll take a while to go through the changes just to ensure it's not adding too much complexity – but your explanations of the changes sound totally reasonable, so it gets a +1 from me so far. Thanks again

JounQin · 2023-12-26T10:47:20Z

Though I have to admit that if possible I would also like to make a PR to remove @pkgr/utils because it brings a lot of dependencies.

@onigoetz Sure, this can be done with a separate PR, or if you think that's the cause of performance impact, feel free to combine them together.

And also, the most dependencies of @pkgr/utils are used by https://github.com/un-ts/pkgr/blob/master/packages/utils/src/browser.ts , I may rethink about how to organize it separately.

JounQin · 2023-12-26T15:42:28Z

@onigoetz After https://github.com/un-ts/synckit/releases/tag/v0.8.8, synckit is much more slimmer now! I tried to bundle it with esbuild --minify, the output file is about 6KB.

See also https://bundlephobia.com/package/[email protected]

The latest benchmark shows the following on my personal MackBook Pro with your fork repository https://github.com/onigoetz/sync-threads:

Load time
┌─────────┬────────────────┬──────────┬────────────────────┬──────────┬─────────┐
│ (index) │   Task Name    │ ops/sec  │ Average Time (ns)  │  Margin  │ Samples │
├─────────┼────────────────┼──────────┼────────────────────┼──────────┼─────────┤
│    0    │    'native'    │ '62,032' │ 16120.651973439064 │ '±1.75%' │  6204   │
│    1    │ 'sync-threads' │ '75,918' │ 13172.065878426312 │ '±1.11%' │  7592   │
│    2    │   'synckit'    │ '78,921' │ 12670.74572221417  │ '±1.12%' │  7893   │
└─────────┴────────────────┴──────────┴────────────────────┴──────────┴─────────┘
Execution time
┌─────────┬────────────────┬───────────┬───────────────────┬──────────┬─────────┐
│ (index) │   Task Name    │  ops/sec  │ Average Time (ns) │  Margin  │ Samples │
├─────────┼────────────────┼───────────┼───────────────────┼──────────┼─────────┤
│    0    │    'native'    │ '112,093' │ 8921.161668838718 │ '±1.34%' │  11210  │
│    1    │ 'sync-threads' │ '12,855'  │ 77787.16100087247 │ '±2.89%' │  1286   │
│    2    │   'synckit'    │ '15,422'  │ 64838.80041755026 │ '±2.76%' │  1543   │
└─────────┴────────────────┴───────────┴───────────────────┴──────────┴─────────┘
Total time
┌─────────┬────────────────┬──────────┬───────────────────┬──────────┬─────────┐
│ (index) │   Task Name    │ ops/sec  │ Average Time (ns) │  Margin  │ Samples │
├─────────┼────────────────┼──────────┼───────────────────┼──────────┼─────────┤
│    0    │    'native'    │ '44,037' │ 22708.05823881446 │ '±1.27%' │  4404   │
│    1    │ 'sync-threads' │ '11,252' │ 88866.09055544023 │ '±1.88%' │  1129   │
│    2    │   'synckit'    │ '12,674' │ 78897.37910370721 │ '±1.89%' │  1268   │
└─────────┴────────────────┴──────────┴───────────────────┴──────────┴─────────┘

After:

Load time
┌─────────┬────────────────┬───────────┬────────────────────┬──────────┬─────────┐
│ (index) │   Task Name    │  ops/sec  │ Average Time (ns)  │  Margin  │ Samples │
├─────────┼────────────────┼───────────┼────────────────────┼──────────┼─────────┤
│    0    │    'native'    │ '748,896' │ 1335.2982877952772 │ '±2.21%' │  74890  │
│    1    │ 'sync-threads' │ '822,463' │ 1215.860019692917  │ '±1.04%' │  82247  │
│    2    │   'synckit'    │ '827,253' │ 1208.8188204858584 │ '±2.29%' │  82726  │
└─────────┴────────────────┴───────────┴────────────────────┴──────────┴─────────┘
Execution time
┌─────────┬────────────────┬───────────┬───────────────────┬──────────┬─────────┐
│ (index) │   Task Name    │  ops/sec  │ Average Time (ns) │  Margin  │ Samples │
├─────────┼────────────────┼───────────┼───────────────────┼──────────┼─────────┤
│    0    │    'native'    │ '130,004' │ 7692.029733638031 │ '±1.36%' │  13001  │
│    1    │ 'sync-threads' │ '15,205'  │ 65766.06558781561 │ '±2.13%' │  1521   │
│    2    │   'synckit'    │ '17,416'  │ 57415.75409056262 │ '±2.36%' │  1742   │
└─────────┴────────────────┴───────────┴───────────────────┴──────────┴─────────┘
Total time
┌─────────┬────────────────┬───────────┬───────────────────┬──────────┬─────────┐
│ (index) │   Task Name    │  ops/sec  │ Average Time (ns) │  Margin  │ Samples │
├─────────┼────────────────┼───────────┼───────────────────┼──────────┼─────────┤
│    0    │    'native'    │ '109,904' │ 9098.772339495044 │ '±1.27%' │  10991  │
│    1    │ 'sync-threads' │ '16,748'  │   59706.3490526   │ '±1.29%' │  1675   │
│    2    │   'synckit'    │ '18,256'  │ 54775.76927248254 │ '±1.39%' │  1826   │
└─────────┴────────────────┴───────────┴───────────────────┴──────────┴─────────┘

package.json

onigoetz · 2024-01-03T10:06:41Z

I made some more changes to this PR:

Use a single SharedArrayBuffer (this was a bug initially, but I found a way to get it to work fine)
Add an option to automatically grow the buffer if possible (it is a feature of Node.js 20)
Change the library to run benchmarks for a library that's more precise. also includes memory usage

The incredibly high memory usage of sync-threads 1.0.1 skewed the initial benchmark results, here are the updated numbers which run sync-threads 1.0.1 last to not consume all the memory before (as it would use disk swapping for memory operations, thus making them appear slower than they are)

[initial] Memory usage 58.671104MB (+58.671104MB)
Load time
[after native] Memory usage 97.222656MB (+38.551552MB)
[after sync-threads (PR #3)] Memory usage 90.60352MB (-6.619136MB)
[after synckit] Memory usage 116.588544MB (+25.985024MB)
[after synckit (PR #154)] Memory usage 123.748352MB (+7.159808MB)
[after sync-threads (1.0.1)] Memory usage 125.894656MB (+2.146304MB)

Fastest is native
┌─────────┬────────────────────────┬─────────────┬───────────┬─────────┐
│ (index) │       Task Name        │   ops/sec   │  Margin   │ Samples │
├─────────┼────────────────────────┼─────────────┼───────────┼─────────┤
│    0    │        'native'        │ '5,284,942' │ '± 0.19%' │   97    │
│    1    │       'synckit'        │ '5,033,051' │ '± 0.55%' │   96    │
│    2    │ 'sync-threads (PR #3)' │ '4,935,344' │ '± 0.28%' │   100   │
│    3    │  'synckit (PR #154)'   │ '4,927,874' │ '± 0.26%' │   99    │
│    4    │ 'sync-threads (1.0.1)' │ '4,628,793' │ '± 0.23%' │   97    │
└─────────┴────────────────────────┴─────────────┴───────────┴─────────┘
Execution time
[after native] Memory usage 136.23296MB (+10.338304MB)
[after sync-threads (PR #3)] Memory usage 159.82592MB (+23.59296MB)
[after synckit] Memory usage 204.111872MB (+44.285952MB)
[after synckit (PR #154)] Memory usage 233.783296MB (+29.671424MB)
[after sync-threads (1.0.1)] Memory usage 6784.581632MB (+6550.798336MB)

Fastest is native
┌─────────┬────────────────────────┬───────────┬────────────┬─────────┐
│ (index) │       Task Name        │  ops/sec  │   Margin   │ Samples │
├─────────┼────────────────────────┼───────────┼────────────┼─────────┤
│    0    │        'native'        │ '150,795' │ '± 0.30%'  │   100   │
│    1    │  'synckit (PR #154)'   │ '15,963'  │ '± 0.35%'  │   91    │
│    2    │ 'sync-threads (PR #3)' │ '15,672'  │ '± 0.29%'  │   96    │
│    3    │       'synckit'        │ '15,672'  │ '± 0.35%'  │   99    │
│    4    │ 'sync-threads (1.0.1)' │   '397'   │ '± 37.23%' │   11    │
└─────────┴────────────────────────┴───────────┴────────────┴─────────┘

--- in a separate run ---

Total time
[after native] Memory usage 134.02112MB (+79.396864MB)
[after sync-threads (PR #3)] Memory usage 153.10848MB (+19.08736MB)
[after synckit] Memory usage 195.264512MB (+42.156032MB)
[after synckit (PR #154)] Memory usage 213.041152MB (+17.77664MB)
[after sync-threads (1.0.1)] Memory usage 7179.780096MB (+6966.738944MB)

Fastest is native
┌─────────┬────────────────────────┬───────────┬────────────┬─────────┐
│ (index) │       Task Name        │  ops/sec  │   Margin   │ Samples │
├─────────┼────────────────────────┼───────────┼────────────┼─────────┤
│    0    │        'native'        │ '144,937' │ '± 0.94%'  │   95    │
│    1    │  'synckit (PR #154)'   │ '16,094'  │ '± 0.37%'  │   95    │
│    2    │ 'sync-threads (PR #3)' │ '15,988'  │ '± 0.38%'  │   98    │
│    3    │       'synckit'        │ '15,506'  │ '± 1.74%'  │   93    │
│    4    │ 'sync-threads (1.0.1)' │   '379'   │ '± 40.76%' │   11    │
└─────────┴────────────────────────┴───────────┴────────────┴─────────┘

JounQin · 2024-01-04T06:25:06Z

@onigoetz Awesome work again, and we can add benchmarks on CI like

https://github.com/un-ts/synckit/blob/68ae0604aeefd9514bb50d61afd9e04a8f7084be/.github/workflows/ci.yml#L41-L43

It would show different benchmark results on different platforms and node versions.

See also https://github.com/un-ts/synckit/actions/runs/7405983872/job/20149716081#step:6:8

┌───────────┬────────────┬──────────────┬───────────────────┬─────────────┬────────────────┬───────────┬────────────────┐
│  (index)  │  synckit   │ sync-threads │ perf sync-threads │   deasync   │  perf deasync  │  native   │  perf native   │
├───────────┼────────────┼──────────────┼───────────────────┼─────────────┼────────────────┼───────────┼────────────────┤
│ loadTime  │ '13.02ms'  │   '1.98ms'   │  '6.56x slower'   │  '1.72ms'   │ '7.58x slower' │ '22.53ms' │ '1.73x faster' │
│  runTime  │ '189.66ms' │ '22844.97ms' │ '120.45x faster'  │ '1593.37ms' │ '8.40x faster' │ '27.19ms' │ '6.97x slower' │
│ totalTime │ '202.68ms' │ '22846.96ms' │ '112.72x faster'  │ '1595.09ms' │ '7.87x faster' │ '49.72ms' │ '4.08x slower' │
└───────────┴────────────┴──────────────┴───────────────────┴─────────────┴────────────────┴───────────┴────────────────┘

onigoetz added 9 commits December 21, 2023 10:58

Add a test

a65b30d

Fix issue that prevents from using a sync function more than once

a5a22c4

Add test to ensure errors are forwarded

5a2c2eb

Support multiple arguments

1252ef8

Add support for timeouts

7e7effe

Improve timeout test

0e7dc3f

Reuse a single worker for all function calls

49d92db

Add benchmark

93f7993

Fix benchmark title

d008ba8

onigoetz mentioned this pull request Dec 22, 2023

It is somehow slower than synckit and can be faster #2

Open

properly retrieve custom fields from errors

c61c82b

This was referenced Dec 26, 2023

split @pkgr/utils into @pkgr/core and @pkgr/browser, etc un-ts/pkgr#352

Closed

feat: migrate @pkgr/utils to lite @pkgr/core un-ts/synckit#148

Merged

onigoetz added 2 commits January 3, 2024 01:19

Use Uint8Array instead of Buffer.from

07517c1

Initialize a single SharedArrayBuffer

0ce2d6d

JounQin reviewed Jan 3, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

onigoetz added 4 commits January 3, 2024 10:38

Automatically grow the buffer if possible

f8ae557

Move tests to a separate folder

c1c83c5

Generate latest types

d630f24

Use benchmark instead of tinybench, update dependencies

bc107c1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix and performance improvements#3

Bugfix and performance improvements#3
onigoetz wants to merge 16 commits intolambci:masterfrom
onigoetz:master

onigoetz commented Dec 22, 2023 •

edited

Loading

Uh oh!

JounQin commented Dec 26, 2023 •

edited

Loading

Uh oh!

onigoetz commented Dec 26, 2023

Uh oh!

JounQin commented Dec 26, 2023

Uh oh!

onigoetz commented Dec 26, 2023

Uh oh!

mhart commented Dec 26, 2023

Uh oh!

JounQin commented Dec 26, 2023 •

edited

Loading

Uh oh!

JounQin commented Dec 26, 2023 •

edited

Loading

Uh oh!

Uh oh!

onigoetz commented Jan 3, 2024 •

edited

Loading

Uh oh!

JounQin commented Jan 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

onigoetz commented Dec 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JounQin commented Dec 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

onigoetz commented Dec 26, 2023

Uh oh!

JounQin commented Dec 26, 2023

Uh oh!

onigoetz commented Dec 26, 2023

Uh oh!

mhart commented Dec 26, 2023

Uh oh!

JounQin commented Dec 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JounQin commented Dec 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

onigoetz commented Jan 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JounQin commented Jan 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

onigoetz commented Dec 22, 2023 •

edited

Loading

JounQin commented Dec 26, 2023 •

edited

Loading

JounQin commented Dec 26, 2023 •

edited

Loading

JounQin commented Dec 26, 2023 •

edited

Loading

onigoetz commented Jan 3, 2024 •

edited

Loading