Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: daemon and cid optimizations #28

Merged
merged 7 commits into from
May 5, 2022
Merged

fix: daemon and cid optimizations #28

merged 7 commits into from
May 5, 2022

Conversation

SgtPooki
Copy link
Member

fixes #25

@SgtPooki SgtPooki requested a review from lidel April 25, 2022 20:44
@SgtPooki SgtPooki self-assigned this Apr 25, 2022
@SgtPooki SgtPooki changed the title fix: Use existing client if one is running fix: Use existing daemon if one is running Apr 25, 2022
This was linked to issues Apr 25, 2022
Support running via npx #22
Closed
Bug: Error when IPFS-desktop is running bind: address already in use #25
Closed
lidel
lidel requested changes Apr 26, 2022
View reviewed changes
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

  • Inline CIDs creation can be done in pure JS – see comment inline
  • Avoid reusing daemon if present – tests should use a clean, controlled instance (especially when we will use it for bitswap / DHT tests)

src/utils/getInlineCid.ts Outdated Show resolved Hide resolved
src/utils/getIpfsClient.ts Outdated Show resolved Hide resolved
src/utils/getIpfsClient.ts Outdated Show resolved Hide resolved
@lidel lidel mentioned this pull request Apr 26, 2022
Closed
@SgtPooki
Copy link
Member Author

  • Avoid reusing daemon if present – tests should use a clean, controlled instance (especially when we will use it for bitswap / DHT tests)

So instead of using existing client if one exists, would swapping ports work?

@SgtPooki
fix: Use a clean controlled ipfsd-ctl daemon
8030b93
@SgtPooki
fix: Create inline CID with multiformats
bd2a3c2 
Do not use a daemon for creating an inline CID

Using the following snippet,
```typescript
(async () => {
  try {
    const inlineCid = await getInlineCid()
    console.log(inlineCid)
  } catch (err) {
    console.error(err)
  }
})().catch((err) => {
  console.error(err)
})
```

we see that multiformats is easily 9x faster:

```bash
> hyperfine 'ts-node src/utils/getInlineCid-daemon.ts' 'ts-
node src/utils/getInlineCid-multiformats.ts'
Benchmark 1: ts-node src/utils/getInlineCid-daemon.ts
  Time (mean ± σ):      3.056 s ±  0.187 s    [User: 1.154 s, System: 0.260 s]
  Range (min … max):    2.922 s …  3.405 s    10 runs

Benchmark 2: src/utils/getInlineCid-multiformats.ts
  Time (mean ± σ):     314.9 ms ±   2.8 ms    [User: 319.3 ms, System: 41.3 ms]
  Range (min … max):   310.9 ms … 320.3 ms    10 runs

Summary
  'src/utils/getInlineCid-multiformats.ts' ran
    9.71 ± 0.60 times faster than 'ts-node src/utils/getInlineCid-daemon.ts'
```
@SgtPooki SgtPooki changed the title fix: Use existing daemon if one is running fix: daemon and cid optimizations Apr 27, 2022
achingbrain
achingbrain requested changes Apr 27, 2022
View reviewed changes
CHANGELOG.md Show resolved Hide resolved
package.json Show resolved Hide resolved
src/utils/getInlineCid.ts Outdated Show resolved Hide resolved
src/utils/getIpfsClient.ts Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
SgtPooki
SgtPooki commented May 5, 2022
View reviewed changes
Copy link
Member Author

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

No review for a while, I'm going to merge and we can discuss later if necessary.

@SgtPooki SgtPooki merged commit fc82f5a into addMoreChecks May 5, 2022
@SgtPooki SgtPooki deleted the fix-bug-25 branch May 5, 2022 15:25
SgtPooki added a commit that referenced this pull request May 9, 2022
@SgtPooki @lidel
feat: implement all compliance checks (#17)
1223831 
* feat: check compliance for adding pin

fix #5

* feat: delete all pins

fix #4

* feat: add pagination check (in progress)

* feat: finishing up pagination test

* minor improvements

* feat: finish pagination compliance check

fix #6

* feat: handle rate-limiting in middleware

* feat: Check add+delete pin compliance

fix #7

* update instructions

* feat: Check replacement of pin

fix #8

* docs: Add launch.json

launch.json helps with debugging, but token values are protected against committing by .gitignoring the `.vscode/launch.json` while committing `.vscode/launch-copy.json` instead.

* feat: output reports to markdown files

* updating utils

* making progress on finalized architecture

* create report summary

* add logger and fix deleteAllPins check

* clean up expectation output

* migrate replacePin check to new ApiCall structure

* migrate getAllPins check to use ApiCall class

* add matchPin compliance check

* use ApiCall class in testPagination check

* most checks are done, but currently reaching heap limit

* chore: cleaning up errors and last-minute changes

* update pinning-service-client

* update gh-pages action

* feat: add CodeQL GH action

* chore: add package-lock.json

'npm i' is neither stable/deterministic nor fast.
'npm ci' should be available for developers who do not change any deps.

* docs: cleanup README

* remove editor-specific config files

* remove editorial comment from output markdown

* feat: page publish action uses matrix strategy

* fix: setup does not generate joi spec json files

* fix: exit with error code on failure

* feat: default output is more readable

* fix: serviceAndToken option doesn't block other scripts

* tests are not implemented yet

* fix: don't clobber docs dir

* marked is required for formatting verbose console output

* chore: update contributors

* chore: release version v0.0.2

* chore: simplify ci workflow

- no need to run test:* -- we have none
- 'build' is out test
- README cleanup (act is useless for naything more advanced etc)

* docs: working cli for local dev

* fix: daemon and cid optimizations (#28)

* fix: Use existing client if one is running

fixes #25

* chore: update contributors

* chore: release version v0.0.3

* fix: Use a clean controlled ipfsd-ctl daemon

* fix: Create inline CID with multiformats

Do not use a daemon for creating an inline CID

Using the following snippet,
```typescript
(async () => {
  try {
    const inlineCid = await getInlineCid()
    console.log(inlineCid)
  } catch (err) {
    console.error(err)
  }
})().catch((err) => {
  console.error(err)
})
```

we see that multiformats is easily 9x faster:

```bash
> hyperfine 'ts-node src/utils/getInlineCid-daemon.ts' 'ts-
node src/utils/getInlineCid-multiformats.ts'
Benchmark 1: ts-node src/utils/getInlineCid-daemon.ts
  Time (mean ± σ):      3.056 s ±  0.187 s    [User: 1.154 s, System: 0.260 s]
  Range (min … max):    2.922 s …  3.405 s    10 runs

Benchmark 2: src/utils/getInlineCid-multiformats.ts
  Time (mean ± σ):     314.9 ms ±   2.8 ms    [User: 319.3 ms, System: 41.3 ms]
  Range (min … max):   310.9 ms … 320.3 ms    10 runs

Summary
  'src/utils/getInlineCid-multiformats.ts' ran
    9.71 ± 0.60 times faster than 'ts-node src/utils/getInlineCid-daemon.ts'
```

* fix: correct getInlineCID error output

* chore: tsconfig correctio
ns

Co-authored-by: Marcin Rataj <lidel@lidel.org>
github-actions bot pushed a commit that referenced this pull request Jun 14, 2022
@semantic-release-bot
chore(release): 1.0.0 [skip ci]
89f9a25 
## 1.0.0 (2022-06-14)

### Features

* add npm bin to devcontainer PATH ([365c142](365c142))
* auto-update github actions with dependabot ([#43](#43)) ([87f7926](87f7926))
* compliance check infrastructure ([7aa5663](7aa5663))
* Export esm module ([#41](#41)) ([acaeac6](acaeac6))
* implement all compliance checks ([#17](#17)) ([1223831](1223831)), closes [#5](#5) [#4](#4) [#6](#6) [#7](#7) [#8](#8) [#28](#28) [#25](#25)
* lplaceholder checks run via listr ([bbb1f81](bbb1f81))
* Publish static reports via github pages ([#68](#68)) ([5a6a7f5](5a6a7f5)), closes [#78](#78) [#76](#76) [#77](#77) [#77](#77)

### Bug Fixes

* CI release succeeds ([#90](#90)) ([810d4a2](810d4a2))
* Compliance check sum is consistent for all services ([#54](#54)) ([ace2b57](ace2b57)), closes [#55](#55)
* devcontainer sees pinning-service-client ([c7a6dbb](c7a6dbb))

### Trivial Changes

* **deps-dev:** bump @types/node from 17.0.25 to 17.0.34 ([#49](#49)) ([a2a5d13](a2a5d13))
* **deps-dev:** bump @types/node from 17.0.35 to 17.0.41 ([#79](#79)) ([db8a6f7](db8a6f7))
* **deps-dev:** bump aegir from 37.0.15 to 37.2.0 ([#86](#86)) ([df4035f](df4035f))
* **deps-dev:** bump ipfs-core-types from 0.10.3 to 0.11.0 ([#64](#64)) ([e623ad3](e623ad3))
* **deps-dev:** bump ts-node from 10.7.0 to 10.8.1 ([#75](#75)) ([ff48f3b](ff48f3b))
* **deps:** bump actions/checkout from 2 to 3 ([#46](#46)) ([8fbbcd8](8fbbcd8))
* **deps:** bump actions/setup-node from 2 to 3 ([#48](#48)) ([ee57c59](ee57c59))
* **deps:** bump github/codeql-action from 1 to 2 ([#47](#47)) ([1dd488d](1dd488d))
* **deps:** bump go-ipfs from 0.12.2 to 0.13.0 ([#80](#80)) ([baaaa8c](baaaa8c))
* **deps:** bump ipfsd-ctl from 10.0.6 to 11.0.1 ([#58](#58)) ([45771db](45771db))
* **deps:** bump lewagon/wait-on-check-action from 0.2 to 1.1.1 ([#44](#44)) ([9c8f5c0](9c8f5c0))
* **deps:** bump node-fetch from 3.2.4 to 3.2.6 ([#81](#81)) ([5bcb760](5bcb760))
* **deps:** bump pascalgn/automerge-action from 0.13.1 to 0.15.3 ([#45](#45)) ([65c19dc](65c19dc))
* getting started instructions ([b7ff148](b7ff148))
* ignore .envrc ([54e5aae](54e5aae))
* static report landing page provides context ([#87](#87)) ([12fc841](12fc841))
* use gitignore.io ([e81ebd9](e81ebd9))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lidel lidel Awaiting requested review from lidel

@achingbrain achingbrain Awaiting requested review from achingbrain

Assignees

@SgtPooki SgtPooki

Labels
need/maintainer-input P1 status/ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: Error when IPFS-desktop is running bind: address already in use Support running via npx
3 participants
@SgtPooki @lidel @achingbrain