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.

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

added first draft of profiler docs #111

Merged
merged 8 commits into from
Jul 21, 2023
Merged

added first draft of profiler docs #111

merged 8 commits into from
Jul 21, 2023

Conversation

pigmej
Copy link
Member

@pigmej pigmej commented Jul 21, 2023

Added some guides about profiler tool.

@pigmej pigmej requested review from poszu and fasmat July 21, 2023 12:17
@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

Merging #111 (a8a32f7) into main (95c4af2) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #111   +/-   ##
=======================================
  Coverage   97.56%   97.56%           
=======================================
  Files          13       13           
  Lines        1601     1601           
=======================================
  Hits         1562     1562           
  Misses         39       39           

docs/profiler.md Outdated Show resolved Hide resolved
docs/profiler.md Outdated Show resolved Hide resolved
docs/profiler.md Outdated Show resolved Hide resolved
docs/profiler.md Outdated Show resolved Hide resolved
docs/profiler.md Outdated Show resolved Hide resolved
docs/profiler.md Outdated
Comment on lines 101 to 110
## How to use the values

```
"smeshing-proving-opts": {
"smeshing-opts-proving-nonces": 144,
"smeshing-opts-proving-threads": 0
},
```

Place that in your node config. Please note that the values are just an example and you need to use your own values.
Copy link
Collaborator

Choose a reason for hiding this comment

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

People will not know where to place it in the config. Consider specifying the whole path in JSON:

{
  "smeshing": {
    "smeshing-proving-opts": {
      "smeshing-opts-proving-nonces": 144,
      "smeshing-opts-proving-threads": 0
    }
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure honestly, that would assume that people know how to merge json. I gave flat part of a file that they have to add (including semicolon at the end)

Copy link
Collaborator

@poszu poszu Jul 21, 2023

Choose a reason for hiding this comment

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

OK, but where to put it? I had to open source code and look it up to write the proposed version 😂

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm ok you convinced me. I'll add "smashing" also :) but I'll drop the top most {} ok?

Copy link
Member Author

Choose a reason for hiding this comment

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

@poszu check now, changed my mind a bit and tried to make it more explicit.

docs/profiler.md Outdated Show resolved Hide resolved
docs/profiler.md Outdated Show resolved Hide resolved
docs/profiler.md Outdated Show resolved Hide resolved
docs/profiler.md Outdated Show resolved Hide resolved
docs/profiler.md Outdated Show resolved Hide resolved
docs/profiler.md Outdated Show resolved Hide resolved
docs/profiler.md Outdated Show resolved Hide resolved
docs/profiler.md Outdated Show resolved Hide resolved
@poszu
Copy link
Collaborator

poszu commented Jul 21, 2023

Content-wise LGTM, but it could be restructured and better worded.

Do we really want to put exact mainnet values (like 12h cycle gap, network id) here? I see that it can help people right now, but could be misleading if they change (the cycle gap for sure will change at some point).

docs/profiler.md Outdated Show resolved Hide resolved
docs/profiler.md Outdated Show resolved Hide resolved
docs/profiler.md Outdated Show resolved Hide resolved
docs/profiler.md Outdated Show resolved Hide resolved
docs/profiler.md Outdated Show resolved Hide resolved
docs/profiler.md Outdated Show resolved Hide resolved
docs/profiler.md Outdated Show resolved Hide resolved
docs/profiler.md Outdated Show resolved Hide resolved
docs/profiler.md Outdated Show resolved Hide resolved
docs/profiler.md Outdated Show resolved Hide resolved
pigmej and others added 2 commits July 21, 2023 15:13
Co-authored-by: Matthias Fasching <[email protected]>
Co-authored-by: Bartosz Różański <[email protected]>
@fasmat fasmat self-requested a review July 21, 2023 14:51
@pigmej pigmej merged commit 16c6ea0 into main Jul 21, 2023
17 checks passed
@fasmat fasmat deleted the add_profiler_docs branch July 26, 2023 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants