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

build: allow easier checking of permanent deoptimizations #12456

Merged
merged 13 commits into from Apr 30, 2017

Conversation

@mscdex
Copy link
Contributor

commented Apr 17, 2017

As the title says, this adds a build target to allow for checking for permanent deoptimizations in node core simply by running the parallel and sequential tests. This is not a "complete" solution per-se because it currently creates (and ignores) the issues it causes with tests that fork processes due to the extra output on stdout, which some tests check. However, aside from that it should work correctly everywhere else.

There are a few deoptimization fixes I've left out in this PR:

  • domain.js

    • runBound() and runIntercepted() have deopts that do not seem straight-forward to fix AFAICT because of the API we are exposing. The deopt reason for both is:

      Bad value context for arguments value

      I was able to fix this same issue in another spot by bind()ing the function (this makes it monomorphic), but I'm not sure we can do that in these two functions because this is being utilized and I am not sure how much of userland calls the created bound/intercept functions with custom this values.

  • internal/process.js

    • v8BreakIterator() also has
      Bad value context for arguments value
      I'm ignoring it since it's deprecated.
  • punycode.js

    • adapt() and decode() both have:
      Unsupported let compound assignment
      I'm ignoring these because it's third party code and the module is deprecated.
  • repl.js

    • This is a strange one and I haven't had time to narrow it down since the deopt message does not include a function name, but the reason is:
      Invalid left-hand side in assignment
      I've read that this may be due to a Crankshaft limitation and that it's *possible* TurboFan may not have this same deopt issue.

There are a bunch of other permanent deopts, but they lie in the actual tests themselves and not in lib/. Currently this build target shows all deopts, no matter where they occur as it does not seem that we can always reliably detect whether a deopt is from a test or not. I am not sure if deopts in tests are worth going after, perhaps it could be a possibility for code-and-learn or similar events (although the deopt reasons may be too confusing for some audiences)?

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

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)
  • build
  • lib
  • test
@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2017

The changes in fs might be considered semver-major-y because of the change in how the callback is detected. The new method of callback detection I used is also used in other parts of fs though FWIW.

@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2017

Also, I'm not sure if the vcbuild.bat changes are absolutely correct or done the best way, batch files are not my forte.

For *nix though all you need to do is make test-check-deopts to run the checks.

@mscdex mscdex force-pushed the mscdex:build-check-deopts branch 2 times, most recently Apr 17, 2017

@mscdex mscdex referenced this pull request Apr 17, 2017
2 of 2 tasks complete
@benjamingr

This comment has been minimized.

Copy link
Member

commented Apr 17, 2017

Great work! I think this is a good step and I'd love to see this as part of our build.

I am not sure if deopts in tests are worth going after, perhaps it could be a possibility for code-and-learn or similar events (although the deopt reasons may be too confusing for some audiences)?

It depends on whether or not they're actually in the tests or are caused by code calling code outside of the tests and would happen in user code when calling Node modules too necessarily.


I'd like to see how this looks on TF+Ignition. Will review actual code soon.

lib/fs.js Outdated
for (const key in source)
var target = {};
var key;
for (key in source)

This comment has been minimized.

Copy link
@benjamingr

benjamingr Apr 17, 2017

Member

Does for (var key in source) really deopt here?

This comment has been minimized.

Copy link
@mscdex

mscdex Apr 17, 2017

Author Contributor

No, just the const usage. I thought there was an issue with using var in a for-in at one time, but I could be remembering wrong. I will change those.

@@ -320,7 +321,7 @@ fs.existsSync = function(path) {
};

fs.readFile = function(path, options, callback) {
callback = maybeCallback(arguments[arguments.length - 1]);
callback = maybeCallback(callback || options);

This comment has been minimized.

Copy link
@benjamingr

benjamingr Apr 17, 2017

Member

I see why this might indeed be "semver-major-y" but it looks fine to me. The only difference is in the case callback is passed but not as a function at which point options would be passed and would be rethrown if real options passed.

I think a CITGM run would be sufficient here and if that passes I don't think we need to change it or semver-major it.

Also I'm very surprised arguments use only indexed access and length check is performed deopts.

This comment has been minimized.

Copy link
@mscdex

mscdex Apr 17, 2017

Author Contributor

I think it's also because arguments was being used in a function where the named arguments were being overwritten. This could be a Crankshaft-specific limitation though, I'm not sure.

@@ -1248,7 +1247,7 @@ function writeAll(fd, isUserFd, buffer, offset, length, position, callback_) {
}

fs.writeFile = function(path, data, options, callback) {
callback = maybeCallback(arguments[arguments.length - 1]);
callback = maybeCallback(callback || options);

This comment has been minimized.

Copy link
@benjamingr

benjamingr Apr 17, 2017

Member

ditto for comment above.

@@ -99,7 +99,7 @@ function shared(message, handle, indexesKey, cb) {
delete handles[key];
delete indexes[indexesKey];
return close.apply(this, arguments);
};
}.bind(handle);

This comment has been minimized.

Copy link
@benjamingr

benjamingr Apr 17, 2017

Member

Mind explaining this one? Typically bind isn't that great at not causing deopts :D

This comment has been minimized.

Copy link
@mscdex

mscdex Apr 17, 2017

Author Contributor
Bad value context for arguments value

See: https://gist.github.com/Hypercubed/89808f3051101a1a97f3

@benjamingr

This comment has been minimized.

Copy link
Member

commented Apr 17, 2017

Actual changes LGTM but I'd like to check it out and test it first.

I'd also land this in two parts - fixing the deopts and adding the flag, but I'd prefer defer that choice to you and the CTC.

@mscdex mscdex force-pushed the mscdex:build-check-deopts branch Apr 17, 2017

@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2017

It depends on whether or not they're actually in the tests or are caused by code calling code outside of the tests and would happen in user code when calling Node modules too necessarily.

For the most part the deopts in tests are clearly labeled with absolute paths to the test file in the deopt message line, so that was how I knew the deopt was not actually happening in node core. However for some special cases, such as the vm tests where the filename can be overridden, this absolute path does not exist, making it possibly look like a node core deopt.

@refack

This comment has been minimized.

Copy link
Member

commented Apr 17, 2017

How about for all the places you replaced const with var, you add a /** @const */ so the intention is saved, and could be checked by linters?

@refack
Copy link
Member

left a comment

Add /** @const */ where replacing const with var

@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2017

@refack Here are some issues I see with doing such things:

  • It can add a lot of noise, which could especially become a problem for functions designed to be inlineable in Crankshaft
  • Adding comments for one particular type of deopt and not all others doesn't seem particularly useful, and doing it for all deopt situations can be tedious. Combine this with the fact that some of these deopts may not even be a thing anymore after the switch to TurboFan + Ignition.
  • I think there is/was already efforts somewhere for linters to check for potentially deoptimized code without the need for any kind of hints/metadata.

My intention with this PR was to have something so that in case someone does change a var back to const for example, they can run make test-check-deopts afterwards and notice that there is a new deopt and that they should revert their change. This way we don't need to litter the source with unnecessary comments.

@refack

This comment has been minimized.

Copy link
Member

commented Apr 17, 2017

Apart from automatic tools, there's also the future human reader.

It can add a lot of noise, which could especially become a problem for functions designed to be inlineable in Crankshaft

Comments? will add noise for Crankshaft? (sincere question, could you point me to more info?)

Adding comments for one particular type of deopt and not all others doesn't seem particularly useful, and doing it for all deopt situations can be tedious.

var => const seems like the most prevalent in this PR, and is a only one that causes loss of intention.

Combine this with the fact that some of these deopts may not even be a thing anymore after the switch to TurboFan + Ignition.

Not be a thing as in; not be deopts? so we'd revert this change?

I think there is/was already efforts somewhere for linters to check for potentially deoptimized code without the need for any kind of hints/metadata.

I just don't want the intention to be lost (some of your changes superficially seem syntactical, while other are semantic [reusing the vars])

@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2017

@refack

Comments? will add noise for Crankshaft? (sincere question, could you point me to more info?)

One way to cause a function to not be inlineable in Crankshaft is to exceed the maximum source length (default is 600 characters).

var => const seems like the most prevalent in this PR, and is a only one that causes loss of intention.

Fixes for other deopts could be viewed just as unclear (e.g. the .bind() addition in child.js to fix a different type of deopt).

Not be a thing as in; not be deopts? so we'd revert this change?

What I mean is that TurboFan + Ignition may not cause these deopts (they may be Crankshaft-specific), so it may/may not matter for the master branch (and node v8.x) after future V8 upgrades. If they are Crankshaft-specific, then they could be reverted if necessary.

I just don't want the intention to be lost (some of your changes superficially seem syntactical, while other are semantic [reusing the vars])

Any reuse of variables is due to duplicate const variable names. When they are converted to var the linter complains, so the variable definition needs to be lifted out.

@refack

This comment has been minimized.

Copy link
Member

commented Apr 17, 2017

I thought that adding comments will be the perfect balance of gaining performance, and keeping readability/maintainability. If I'm so off target, I'll remove my objection.

One way to cause a function to not be inlineable in Crankshaft is to exceed the maximum source length (default is 600 characters).

Including comments?

What I mean is that TurboFan + Ignition may not cause these deopts (they may be Crankshaft-specific), so it may/may not matter for the master branch (and node v8.x) after future V8 upgrades. If they are Crankshaft-specific, then they could be reverted if necessary.

👍

Any reuse of variables is due to duplicate const variable names. When they are converted to var the linter complains, so the variable definition needs to be lifted out.

Ok. but, they are sill "loss of intention" cases.

@refack

This comment has been minimized.

Copy link
Member

commented Apr 17, 2017

Fixes for other deopts could be viewed just as unclear (e.g. the .bind() addition in child.js to fix a different type of deopt).

In second thought that's actually even worse, as a future dev (or a current dev like me) will get confused and might want to remove it. IMHO non-intuitive changes should be commented.

@refack

This comment has been minimized.

Copy link
Member

commented Apr 17, 2017

It can add a lot of noise, which could especially become a problem for functions designed to be inlineable in Crankshaft

One way to cause a function to not be inlineable in Crankshaft is to exceed the maximum source length (default is 600 characters).

Including comments?

@hashseed do you know if comments hinder inlining?

evanlucas added a commit that referenced this pull request May 2, 2017
cluster: fix permanent deoptimizations
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request May 2, 2017
process: fix permanent deoptimizations
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request May 2, 2017
net: fix permanent deoptimizations
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request May 2, 2017
readline: fix permanent deoptimizations
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request May 2, 2017
test: fix permanent deoptimizations
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request May 2, 2017
test: fix test filenames
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request May 2, 2017
build: add target for checking for perm deopts
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request May 2, 2017
stream: fix permanent deoptimizations
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request May 2, 2017
fs: fix permanent deoptimizations
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request May 2, 2017
cluster: fix permanent deoptimizations
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request May 2, 2017
process: fix permanent deoptimizations
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request May 2, 2017
net: fix permanent deoptimizations
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request May 2, 2017
readline: fix permanent deoptimizations
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request May 2, 2017
test: fix permanent deoptimizations
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request May 3, 2017
test: fix test filenames
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request May 3, 2017
build: add target for checking for perm deopts
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request May 3, 2017
stream: fix permanent deoptimizations
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request May 3, 2017
fs: fix permanent deoptimizations
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request May 3, 2017
cluster: fix permanent deoptimizations
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request May 3, 2017
process: fix permanent deoptimizations
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request May 3, 2017
net: fix permanent deoptimizations
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request May 3, 2017
readline: fix permanent deoptimizations
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request May 3, 2017
test: fix permanent deoptimizations
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@gibfahn

This comment has been minimized.

Copy link
Member

commented May 15, 2017

@mscdex should this be backported to v6.x?

@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2017

@gibfahn Sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.