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

Setup docsify for end user documentation #991

Merged
merged 6 commits into from
May 21, 2018

Conversation

gregtatum
Copy link
Member

@gregtatum gregtatum commented May 9, 2018

This PR sets up docsify for the end-user documentation. I originally tried GitBook, but its static site generator is unmaintained, and I ran into several bugs that were already on file.

There were a few copy edits from the feedback from #983 mixed in the final commit with the setup. I will most likely follow-up with more work on the copy.

Here is a direct link to the deploy preview: https://deploy-preview-991--perf-html.netlify.com/docs/

This PR is in support of #805.

Another follow-up will be to integrate this into the perf.html UI.

@gregtatum gregtatum requested a review from zoepage May 9, 2018 15:22
@codecov
Copy link

codecov bot commented May 9, 2018

Codecov Report

Merging #991 into master will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #991      +/-   ##
==========================================
+ Coverage   70.06%   70.09%   +0.03%     
==========================================
  Files         141      141              
  Lines        8182     8157      -25     
  Branches     1854     1842      -12     
==========================================
- Hits         5733     5718      -15     
+ Misses       2162     2154       -8     
+ Partials      287      285       -2
Impacted Files Coverage Δ
src/utils/window-console.js 93.75% <ø> (ø) ⬆️
src/profile-logic/process-profile.js 88.88% <ø> (ø) ⬆️
src/profile-logic/profile-data.js 76.61% <ø> (ø) ⬆️
src/components/shared/chart/Viewport.js 30.05% <0%> (-0.37%) ⬇️
src/components/flame-graph/FlameGraph.js 61.11% <0%> (ø) ⬆️
src/components/header/IntervalMarkerOverview.js 53.9% <0%> (+0.06%) ⬆️
src/components/shared/CallNodeContextMenu.js 87.8% <0%> (+0.3%) ⬆️
src/components/flame-graph/Canvas.js 66.28% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42ac882...3e068ee. Read the comment docs.

@zoepage
Copy link
Contributor

zoepage commented May 16, 2018

Page not starting on top

When I scroll down one page and click another link in the sidebar, the page view does not start at the top, but the same position I used to be on on the last page.

screen shot 2018-05-16 at 4 03 15 pm

The font in the sidebar navigation seems cut off.

font-size: 14px
screen shot 2018-05-16 at 3 17 34 pm

font-size: 15px
screen shot 2018-05-16 at 3 17 43 pm

Highlight link

Would it be possible to highlight the root of the page as we display the same content in README.md as in \ ?
screen shot 2018-05-16 at 3 15 38 pm
screen shot 2018-05-16 at 3 15 46 pm

mobile view - menu button at bottom

We might want to move the button to the top on mobile view as it probably will be not seen at the bottom.

screen shot 2018-05-16 at 3 15 15 pm

Also the icon has no background, so it overlays the text and is hard to see.
screen shot 2018-05-16 at 4 25 04 pm

mobile view - top navigation

The 384px top navigation is breaking and overlaying the headline.

screen shot 2018-05-16 at 3 15 07 pm

@gregtatum
Copy link
Member Author

Page not starting on top

Fixed with the auto2top configuration item.

The font in the sidebar navigation seems cut off.

I'm not understanding this feedback.

Highlight link

Ah, I figured it out. Should be fixed.

mobile view - top navigation

Fixed

@zoepage
Copy link
Contributor

zoepage commented May 16, 2018

The font in the sidebar navigation seems cut off.

I'm not understanding this feedback.

I'm sorry for not being clear here. The top of e.g. Profiler Fundamentals is cut off, which does not happen on my Retina 13" display, but on my external monitor.


![Screenshot of perf.html](./images/screenshot.png)

Welcome to the user docs for [perf.html](https://perf-html.io), a web app that analyzes profiles from Firefox. [Visit the web app](https://perf-html.io), and follow the instructions to get started profiling. This guide has various documents and videos demonstrating how to get started profiling. Use the links on the left to dive into more details on how to use the product. Looking to contribute? Check out [github.com/devtools-html/perf.html](https://github.com/devtools-html/perf.html).
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the links on the left to dive into more details on how to use the product.
This feels very obvious to me. Maybe we remove that part?

Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently I feel the need to explain how to use a sidebar. :D

@@ -18,7 +18,7 @@ While samples provide a probabilistic view into the profiled program, markers pr

Copy link
Contributor

Choose a reason for hiding this comment

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

The first is to increase the sampling rate.

What do you think about adding how? Just a short addition like ... manually in the settings. or something similar.

Copy link
Contributor

Choose a reason for hiding this comment

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

This can skew the results.
Adding a short line about how maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

The other way to increase the number of samples it run the code more often,

it --> is

Copy link
Contributor

Choose a reason for hiding this comment

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

While samples provide a probabilistic view into the profiled program, markers provide a deterministic view into executing code. Markers are instrumentation that is collected every time an event happens. For instance, when clicking the mouse, the C++ implementation could call a function that collects the current mouse coordinates and timestamp of the event. This information would then be stored in the profiler's buffer.

As a non-native, I kind of had a hard time to follow that paragraph. Maybe changing instrumentation in Markers are instrumentation that is collected every time an event happens. could already loose it up a little.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I rewrote it without the words "probabilistic" and "deterministic".

@@ -18,7 +18,7 @@ While samples provide a probabilistic view into the profiled program, markers pr

Samples are a general-purpose solution to understand what is going on in the code, while markers provide a specific opinionated view. A very fast mouse event will have a high probability of being missed on a low sampling rate, but will always be collected as a marker. The problem with markers is that they must be hand-instrumented in the code, and kept up to date. They are a great way for engineers with domain-specific information to surface more information about how a program is executing. The marker information can be cross-referenced with the samples to provide a better understanding of what code is doing.

Markers can also be used for stack-like purposes as well. For instance when working with React or any front-end component system, the JavaScript stacks will all be framework internals. The internals are resolving work that was request by the components, but they do not directly map to the code that the component author wrote. To get around this, components could emit markers with start and end timestamps for when they are updated. These markers can then be turned into a chart that that shows how components are rendering according to a hierarchy, and how much time is being spent updating them. This labeled information can then be cross referenced with the statistically collected samples.
Markers can also be used for stack-like purposes as well. For instance when working with React or any front-end component system, the JavaScript stacks will all be framework internals. The internals are resolving work requested by the components, but they do not directly map to the code that the component author wrote. To get around this, components could emit markers with start and end timestamps for when they are updated. These markers can then be turned into a chart that shows how components are rendering according to a hierarchy, and how much time is being spent updating them. This labeled information can then be cross referenced with the statistically collected samples.

### Overhead and trade-offs with markers

Copy link
Contributor

Choose a reason for hiding this comment

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

overhead—the sample -> overhead — the sample
Missing some blanks?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've been following the rule that em dashes do not require spaces between them, which looks odd in fixed-width fonts, but should be fine in the final presentation.

@@ -1,32 +1,32 @@
# Stack Samples and Call Trees

The [profiler fundamentals](./profiler-fundamentals) details information about both samples and markers. While samples can collect any type of arbitrary information, this document explores one core idea about samples—that they can collect stacks.
The [profiler fundamentals](./guide-profiler-fundamentals.md) details information about both samples and markers. While samples can collect any type of arbitrary information, this document explores one core idea about samples—that they can collect stacks.
Copy link
Contributor

Choose a reason for hiding this comment

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

The [profiler fundamentals](./guide-profiler-fundamentals.md) details information about both samples and markers.

Feels like there is something missing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that is very poorly worded. I rewrote it.

@gregtatum
Copy link
Member Author

I'm sorry for not being clear here. The top of e.g. Profiler Fundamentals is cut off, which does not happen on my Retina 13" display, but on my external monitor.

Sorry, I'm still not seeing this or properly understanding it.

@gregtatum
Copy link
Member Author

Ok, all of your review comments should be addressed @zoepage

Would you mind updating the status of the review? Generally our practice has been to make the status of the review to be:

  • Approved - The overall PR looks good, some changes may need to be addressed, but the reviewer does not feel the need to double check the changes.
  • Changes requested - Some things need to be changed, and the reviewer would like to check the results before approval, and possibly provide additional feedback. This is typically the case when things are wrong, or the requested changes introduce a lot of new code.
  • Comment only - Either the review is not done, or the comments are to provide additional feedback for some other review. In this case it would probably be good to leave a comment as to the status of the review, such as "I reviewed the approach, and it looks good, I'd like to spend more time tomorrow trying it out locally before approving"

@zoepage
Copy link
Contributor

zoepage commented May 21, 2018

Thank you for the explanation. I'll make sure to be more aligned with it in the future. :)
PR looks good! Thanks for the work.

@gregtatum gregtatum merged commit 323478b into firefox-devtools:master May 21, 2018
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.

2 participants