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

lib: restate the code to use primordials #36552

Merged
merged 1 commit into from
Dec 28, 2020

Conversation

PoojaDurgad
Copy link
Contributor

@PoojaDurgad PoojaDurgad commented Dec 17, 2020

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Can you also replace those please:

if (!octalReg.test(value)) {

(typeof port === 'string' && port.trim().length === 0) ||

lib/internal/validators.js Outdated Show resolved Hide resolved
@PoojaDurgad
Copy link
Contributor Author

PoojaDurgad commented Dec 18, 2020

@PoojaDurgad
Copy link
Contributor Author

@aduh95 - yes, I modified the changes. PTAL

@PoojaDurgad PoojaDurgad added lib / src Issues and PRs related to general changes in the lib or src directory. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Dec 18, 2020
@Trott
Copy link
Member

Trott commented Dec 21, 2020

Commit message should indicate where we are using more primordials. Maybe this?

lib: use more primordials in shared validators

@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 21, 2020
@Trott
Copy link
Member

Trott commented Dec 21, 2020

Removed author ready label. It should not be added until a full CI is started.

@Trott
Copy link
Member

Trott commented Dec 21, 2020

Benchmark Cl : https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/765/

It looks like the benchmark is for assert which is the default in the Jenkins interface. I could be wrong, but I don't think that's the benchmark we want here. I don't believe that uses the internal validators. I'm not sure what benchmark would be impacted by this code, but benchmark/util/type-check.js is worth a lock. If it is a relevant benchmark, then change assert to util in the Jenkins form and put type-check in the filter field.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM if no benchmark or CI issues.

@PoojaDurgad
Copy link
Contributor Author

PoojaDurgad commented Dec 21, 2020

@Trott - Thanks for the review. without fully knowing it, I left the default values in jenkins interface.
Benchmark Cl: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/779/

@PoojaDurgad
Copy link
Contributor Author

Benchmark result : https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/782/console

                                                                                                       confidence  improvement  accuracy (*)   (**)  (***)
14:31:32  util/type-check.js n=100000 argument='false-object' version='js' type='ArrayBufferView'                       -2.78 %       ±5.30% ±7.08%  ±9.28%
14:31:32  util/type-check.js n=100000 argument='false-object' version='js' type='TypedArray'                            -1.37 %       ±3.50% ±4.66%  ±6.07%
14:31:32  util/type-check.js n=100000 argument='false-object' version='js' type='Uint8Array'                      *      6.74 %       ±5.52% ±7.37%  ±9.66%
14:31:32  util/type-check.js n=100000 argument='false-object' version='native' type='ArrayBufferView'                    3.49 %       ±4.72% ±6.28%  ±8.18%
14:31:32  util/type-check.js n=100000 argument='false-object' version='native' type='TypedArray'                         0.36 %       ±4.63% ±6.15%  ±8.01%
14:31:32  util/type-check.js n=100000 argument='false-object' version='native' type='Uint8Array'                        -2.16 %       ±4.36% ±5.80%  ±7.55%
14:31:32  util/type-check.js n=100000 argument='false-primitive' version='js' type='ArrayBufferView'                    -3.30 %       ±5.65% ±7.52%  ±9.79%
14:31:32  util/type-check.js n=100000 argument='false-primitive' version='js' type='TypedArray'                         -2.13 %       ±4.46% ±5.94%  ±7.73%
14:31:32  util/type-check.js n=100000 argument='false-primitive' version='js' type='Uint8Array'                         -1.96 %       ±4.35% ±5.79%  ±7.53%
14:31:32  util/type-check.js n=100000 argument='false-primitive' version='native' type='ArrayBufferView'                -5.02 %       ±6.94% ±9.23% ±12.02%
14:31:32  util/type-check.js n=100000 argument='false-primitive' version='native' type='TypedArray'                      1.92 %       ±5.01% ±6.67%  ±8.69%
14:31:32  util/type-check.js n=100000 argument='false-primitive' version='native' type='Uint8Array'                      4.72 %       ±5.51% ±7.34%  ±9.58%
14:31:32  util/type-check.js n=100000 argument='true' version='js' type='ArrayBufferView'                               -1.16 %       ±5.77% ±7.69% ±10.03%
14:31:32  util/type-check.js n=100000 argument='true' version='js' type='TypedArray'                                     2.32 %       ±6.53% ±8.69% ±11.33%
14:31:32  util/type-check.js n=100000 argument='true' version='js' type='Uint8Array'                                    -0.87 %       ±4.90% ±6.52%  ±8.48%
14:31:32  util/type-check.js n=100000 argument='true' version='native' type='ArrayBufferView'                           -3.66 %       ±4.93% ±6.56%  ±8.53%
14:31:32  util/type-check.js n=100000 argument='true' version='native' type='TypedArray'                                 1.97 %       ±4.33% ±5.77%  ±7.53%
14:31:32  util/type-check.js n=100000 argument='true' version='native' type='Uint8Array'                                 0.84 %       ±4.62% ±6.15%  ±8.00%
14:31:32 
14:31:32 Be aware that when doing many comparisons the risk of a false-positive
14:31:32 result increases. In this case there are 18 comparisons, you can thus
14:31:32 expect the following amount of false-positive results:
14:31:32   0.90 false positives, when considering a   5% risk acceptance (*, **, ***),
14:31:32   0.18 false positives, when considering a   1% risk acceptance (**, ***),
14:31:32   0.02 false positives, when considering a 0.1% risk acceptance (***)

is this performance result within allowable limits?

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 22, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 22, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

lib/internal/validators.js Outdated Show resolved Hide resolved
@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 28, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 28, 2020
@nodejs-github-bot
Copy link
Collaborator

PR-URL: nodejs#36552
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@aduh95 aduh95 merged commit 0538749 into nodejs:master Dec 28, 2020
@aduh95
Copy link
Contributor

aduh95 commented Dec 28, 2020

Landed in 0538749

danielleadams pushed a commit that referenced this pull request Jan 12, 2021
PR-URL: #36552
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@danielleadams danielleadams mentioned this pull request Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants