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.

Sign up for GitHub

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

Jump to bottom

src: start annotating native code side effect #21458

Closed
wants to merge 5 commits into from
Closed

src: start annotating native code side effect #21458

wants to merge 5 commits into from

Conversation

TimothyGu
Copy link
Member

Allows eager evaluation to take place for many builtin modules.

screenshot from 2018-06-22 01-37-13

Refs: #20977

/cc @nodejs/v8-inspector @benjamingr

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@TimothyGu TimothyGu added c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency. inspector Issues and PRs related to the V8 inspector protocol labels Jun 22, 2018
@nodejs-github-bot
Copy link
Collaborator

@TimothyGu build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/117/pipeline

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jun 22, 2018
@TimothyGu
Copy link
Member Author

Full CI: https://ci.nodejs.org/job/node-test-pull-request/15563/

bnoordhuis
bnoordhuis approved these changes Jun 22, 2018
View reviewed changes
src/env-inl.h
NewFunctionTemplate(callback,
v8::Local<v8::Signature>(),
// TODO(TimothyGu): Investigate if SetMethod is ever
// used for constructors.
Copy link
Member

Choose a reason for hiding this comment

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

That's easy to check, isn't it? Or do you suspect make test will miss instances?

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'd rather leave that to a separate PR is what I meant.

src/node_crypto.cc Outdated
env->SetMethod(target, "getHashes", GetHashes);
env->SetMethod(target, "getCurves", GetCurves);
env->SetMethod(target, "randomBytes", RandomBytes,
SideEffectType::kHasNoSideEffect);
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason you annotated randomBytes but not pbkdf2 and scrypt? They all use the thread pool.

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, I didn't realize RandomBytes was async. I'll remove this part then.

bnoordhuis
bnoordhuis reviewed Jun 22, 2018
View reviewed changes
src/node_crypto.cc Outdated
Signature::New(env->isolate(), t));
Signature::New(env->isolate(), t),
// TODO(TimothyGu): should be deny
ConstructorBehavior::kAllow,
Copy link
Member

Choose a reason for hiding this comment

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

Should be /* length */ 0, ConstructorBehavior::kAllow,. Likewise on line 3968.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, good catch. Fixed.

jasnell
jasnell reviewed Jun 22, 2018
View reviewed changes
src/node.cc Outdated
env->SetMethod(process, "getgid", GetGid);
env->SetMethod(process, "getegid", GetEGid);
env->SetMethod(process, "getgroups", GetGroups);
env->SetMethod(process, "getuid", GetUid,
Copy link
Member

Choose a reason for hiding this comment

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

Just a thought... but a env->SetSafeMethod(process, ...) variant that sets SideEfectType::kHasNoSideEffect automatically may be worthwhile.

@TimothyGu
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/15571/

@bnoordhuis
Copy link
Member

SetSafeMethod() is a bad name. Safe how? f you want something short, use SetMethodNSE() - a little cryptic but on point.

We could then later add SetMethodNSFW() for methods you wouldn't want to be seen calling at the office.

@jasnell
Copy link
Member

jasnell commented Jun 23, 2018

I disagree on Safe being bad here... It's a fairly well known idea that "safe" methods have no side effects and NSE conveys no useful information. I think the code comments are enough to convey what Safe means here.

@bnoordhuis
Copy link
Member

It's a fairly well known idea that "safe" methods have no side effects

We have functions such as SafeGetenv() and SafeX509ExtPrint() where the Safe means they go the extra mile to stay secure. It doesn't denote anything about side effects.

@jasnell
Copy link
Member

jasnell commented Jun 23, 2018 •
edited
Loading

I'm not even remotely interested in debating it so whatever method name folks think is appropriate is fine with me. Having the utility method is what I wanted so I'm good with this.

@addaleax
Copy link
Member

and NSE conveys no useful information

@TimothyGu I’d suggest just spelling it out then as NoSideEffect, if that’s alright with you? I agree with Ben that Safe is not a great name, it doesn’t convey much information either.

@TimothyGu
Copy link
Member Author

@addaleax Sure, I'll fix that later.

@benjamingr
Copy link
Member

For what it's worth Pure is typically used to indicate a function has no side effects - at least a lot more often than Safe or NSE. https://www.sitepoint.com/functional-programming-pure-functions/

@bnoordhuis
Copy link
Member

"Pure" suggests it's safe to collapse multiple calls because the return value depends only on the inputs. That clearly isn't true for functions such as process.uptime().

Compare gcc's __attribute__((pure)).

benjamingr
benjamingr approved these changes Jun 24, 2018
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Still looks good after rename

@TimothyGu
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/15623/

@TimothyGu
Copy link
Member Author

Landed in 65f6173.

@TimothyGu TimothyGu closed this Jun 26, 2018
@TimothyGu TimothyGu deleted the side-effects branch June 26, 2018 03:16
TimothyGu added a commit that referenced this pull request Jun 26, 2018
@TimothyGu
src: start annotating native code side effect
65f6173 
PR-URL: #21458
Refs: #20977
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos
Copy link
Member

targos commented Jun 26, 2018

Depends on #21105 to land on v10.x

@targos targos mentioned this pull request Jul 14, 2018
Closed
2 tasks
targos pushed a commit that referenced this pull request Jul 14, 2018
@TimothyGu @targos
src: start annotating native code side effect
51d613d 
PR-URL: #21458
Refs: #20977
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Jul 17, 2018
Merged
This was referenced Jul 18, 2018
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devsnek devsnek devsnek approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@cjihrig cjihrig cjihrig approved these changes

@benjamingr benjamingr benjamingr approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol lib / src Issues and PRs related to general changes in the lib or src directory. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@TimothyGu @nodejs-github-bot @bnoordhuis @jasnell @addaleax @benjamingr @targos @cjihrig @joyeecheung @devsnek