-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
doc: reorganize benchmarking guide/readme, update test/README.md #11237
Conversation
Would like some feedback from @AndreasMadsen on the new benchmark/README.md |
I'm not sure I agree with the hole guide concept as I think it is too hidden, but looking besides that:
|
@AndreasMadsen Thanks for the feedback. I agree the guides are a little bit hidden at the moment, but as we put more documents there hopefully they can get more traction. About the directories, if we make the same changes to test/README.md then a table would be better since they have The |
benchmark/README.md
Outdated
* `_cli.R`: parses the command line arguments passed to `compare.R` | ||
* `_http-benchmarkers.js`: selects and runs external tools for benchmarking | ||
the `http` subsystem. | ||
* `common.js`: see [Common API]((#common-api)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be only one pair of parentheses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there
benchmark/README.md
Outdated
writing JavaScript run by the built-in JavaScript engine. | ||
|
||
For a detailed guide on how to write and run benchmarks in this | ||
directory, see [the guide on benchmarks](../doc/guides/writing-and-running-benchmarks.md). | ||
|
||
## Table of Content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this was already there but s/Content/Contents
benchmark/README.md
Outdated
|
||
## Benchmark Directories | ||
|
||
### arrays |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having these as a table may be more concise. Separate headers doesn't seem necessary
benchmark/README.md
Outdated
``` | ||
Force V8 to optimize the function with the native function | ||
`%OptimizeFunctionOnNextCall()` and return the optimization status | ||
after that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A mention about the (common.v8ForceOptimization()
](https://github.com/nodejs/node/blob/master/benchmark/common.js#L233) utility method would be good here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh, well, this is actually the documentation of that method :S
|
||
## Prerequisites | ||
|
||
Most of the HTTP benchmarks require a benchmarker to be installed, this can be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend putting these under sub-headings.. e.g. ### HTTP Benchmark Requirements
, etc
|
||
### Comparing node versions | ||
|
||
To compare the effect of a new node version use the `compare.js` tool. This |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general throughout this doc, since edits are being made anyway, please s/node/Node.js/g
test/README.md
Outdated
On how to run tests in this direcotry, see | ||
[the contributing guide](../CONTRIBUTING.md#step-5-test). | ||
|
||
## Table of Content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Content/Contents
2397287
to
e18eeb1
Compare
e18eeb1
to
09f96f7
Compare
4f98007
to
fd331c1
Compare
benchmark/README.md
Outdated
|
||
It is used to prevent the benchmark from getting disrupted | ||
by the optimizer kicking in halfway through. However, this could result in a | ||
less effective optimization, so it should be used with caution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actual signature is v8ForceOptimization(method, ...args)
, where args
are passed to method.apply
. Also of note is that method
is executed with this
set to null
. You might also want to explain why it can "result in a less effective optimization."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the reasons for a less effective optimization are probably too many to explain here(no proper inlining because the profiler doesn't know the functions calls inside are hot, no proper type feedback, possible deopt, etc)..probably better left for another PR to update the guide instead of a readme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A general guide would be to grep deps/v8/test
with OptimizeFunctionOnNextCall
to see how it's used though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm..also this doesn't actually optimize the monomorphic pattern method(...args)
if the method has been called with other context/args during the startup, it could be polymorphic or even megamorphic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
..so documenting the null
and ..args
part could be misleading, I am inclined to stay a little bit vague here, if we need to be precise probably it would be best understood by reading its source code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The signature should be corrected though :D
@jasnell @TimothyGu @vsemozhetbyt Thanks for the reviews, updated, PTAL. |
d11bb78
to
b88d113
Compare
@joyeecheung I am not sure if this is a subject for this PR, but could the parameters of the |
@vsemozhetbyt My opinion is that this is a bit out of scope for this pull request. But you are right that it is missing, at the very least we should document how to print the help text. I would also like to consistently have a |
@AndreasMadsen I am not so good in English, so I usually abstain from creating a non-js text in docs. Sorry for not to be helpful here. |
@vsemozhetbyt @AndreasMadsen The current logic in |
I'm aware, but it is not the most intuitive way of getting the help text, it was mostly intended as an input validation. There is also the |
@AndreasMadsen Should we document the current behavior in this PR? I think it's probably better than nothing. If another PR implements |
I like the simple documentation as you have done it now. But as a principal one shouldn't need to look in the documentation in order to learn how to print the help text. |
@jasnell @TimothyGu @vsemozhetbyt @AndreasMadsen Can I have a few LGTM please? |
Do we prefer HTML tables or Markdown tables? Personally I find HTML tables easier to read than Markdown tables when there is a lot of text (markdown lines exceed 80 char). |
I tried the markdown tables first but couldn't find a way to use linebreaks in them, some of the lines are a bit long so I just switched to HTML. I think markdown tables are easier to read when your cells tend to be small and just contain a few words, but not when you want to write actual sentences in them? |
* Write a new benchmark/README.md describing the benchmark directory layout and common API. * Fix the moved benchmarking guide accordingly, add tips about how to get the help text from the benchmarking tools. PR-URL: #11237 Fixes: #11190 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Landed in f8ee197...736814e, thanks! |
* Write a new benchmark/README.md describing the benchmark directory layout and common API. * Fix the moved benchmarking guide accordingly, add tips about how to get the help text from the benchmarking tools. PR-URL: #11237 Fixes: #11190 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
requires a backport PR to land on v4 |
Got some weird merge conflicts..looks like there are ancestor commits not yet backported. |
The benchmark suite runner was rewritten in v7 and because of some issue reports it was decleared a major change. This should not be backported to v4 or v6. |
The current
benchmark/README.md
is in fact a guide on how to run and write benchmarks, this PR moves it to doc/guides (wherewriting-tests.md
is) and create another README.md that explains the directory layout/common API (like whattest/README.md
does). This should make it easier for new contributors to find out what to do when they need to provide a benchmark.Also updates the test/README.md to add a link to the testing guide.
Fixes: #11190
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
doc