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

benchmark should be executed in "strict mode" #3983

Closed
JungMinu opened this issue Nov 23, 2015 · 11 comments
Closed

benchmark should be executed in "strict mode" #3983

JungMinu opened this issue Nov 23, 2015 · 11 comments
Labels
benchmark Issues and PRs related to the benchmark subsystem.

Comments

@JungMinu
Copy link
Member

Files in benchmark are not using strict mode
Strict mode makes it easier to write "secure" JavaScript.
Therefore, benchmark should be executed in strict mode

@silverwind silverwind added the benchmark Issues and PRs related to the benchmark subsystem. label Nov 23, 2015
@silverwind
Copy link
Contributor

+1. Given that most code runs in strict mode nowadays, I think we should benchmark in it too.

@JungMinu
Copy link
Member Author

I will submit a PR soon 😄

@fl0w
Copy link

fl0w commented Nov 23, 2015

What's the difference in just running benchmarks with --use_strict rather? I imagine benchmarking can be done in both modes?

@JungMinu
Copy link
Member Author

@fl0w just running benchmarks with --use_strict will fail because there are lots of variables that are not declared
For instance, in https://github.com/nodejs/node/blob/master/benchmark/idle_clients.js
net = require('net');
Therefore, a PR is necessary

@silverwind
Copy link
Contributor

We don't even lint code in benchmark yet. Running

node tools/eslint/bin/eslint.js benchmark --rulesdir tools/eslint-rules --reset

shows 275 linter errors in benchmark, so it'd be quite a bit of work.

(also, the require-buffer rule can be disabled through a .eslintrc in benchmark. this circular dependency issue should be only relevant in lib)

@Fishrock123
Copy link
Contributor

Note: I think not all of the benchmarks even work anymore.

@JungMinu
Copy link
Member Author

@Fishrock123 I agree, there are many invalid files

@r-52
Copy link
Contributor

r-52 commented Nov 23, 2015

@JungMinu Looks like some older benchmarks use also deprecated methods

@Fishrock123
Copy link
Contributor

Some notes on benchmarks:

@evanlucas
Copy link
Contributor

PR: #5336

@Trott
Copy link
Member

Trott commented Feb 22, 2016

Fixed in #5336

@Trott Trott closed this as completed Feb 22, 2016
Trott added a commit to Trott/io.js that referenced this issue Feb 22, 2016
The lint rule is there to avoid a circular-dependency issue that only
applies to `/lib`. In preparation for linting `/benchmark`, apply that
rule to `/lib` only to avoid churn in `/benchmark`.

Refs: nodejs#3983 (comment)
Trott added a commit to Trott/io.js that referenced this issue Feb 25, 2016
The lint rule is there to avoid a circular-dependency issue that only
applies to `/lib`. In preparation for linting `/benchmark`, apply that
rule to `/lib` only to avoid churn in `/benchmark`.

Refs: nodejs#3983 (comment)
PR-URL: nodejs#5371
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
rvagg pushed a commit that referenced this issue Feb 27, 2016
The lint rule is there to avoid a circular-dependency issue that only
applies to `/lib`. In preparation for linting `/benchmark`, apply that
rule to `/lib` only to avoid churn in `/benchmark`.

Refs: #3983 (comment)
PR-URL: #5371
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
rvagg pushed a commit that referenced this issue Feb 27, 2016
The lint rule is there to avoid a circular-dependency issue that only
applies to `/lib`. In preparation for linting `/benchmark`, apply that
rule to `/lib` only to avoid churn in `/benchmark`.

Refs: #3983 (comment)
PR-URL: #5371
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins pushed a commit that referenced this issue Mar 17, 2016
The lint rule is there to avoid a circular-dependency issue that only
applies to `/lib`. In preparation for linting `/benchmark`, apply that
rule to `/lib` only to avoid churn in `/benchmark`.

Refs: #3983 (comment)
PR-URL: #5371
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins pushed a commit that referenced this issue Mar 21, 2016
The lint rule is there to avoid a circular-dependency issue that only
applies to `/lib`. In preparation for linting `/benchmark`, apply that
rule to `/lib` only to avoid churn in `/benchmark`.

Refs: #3983 (comment)
PR-URL: #5371
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
This was referenced Mar 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem.
Projects
None yet
Development

No branches or pull requests

7 participants