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

@mscdex mscdex 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 mscdex added build Issues and PRs related to build files or the CI. lib / src Issues and PRs related to general changes in the lib or src directory. test Issues and PRs related to the tests. labels Apr 17, 2017
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. lib / src Issues and PRs related to general changes in the lib or src directory. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform. labels Apr 17, 2017
@mscdex mscdex removed the windows Issues and PRs related to the Windows platform. label Apr 17, 2017
@mscdex
Copy link
Contributor Author

mscdex 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
Copy link
Contributor Author

mscdex 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 build-check-deopts branch 2 times, most recently from 9585e11 to 0481405 Compare April 17, 2017 06:20
@benjamingr
Copy link
Member

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)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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);
Copy link
Member

Choose a reason for hiding this comment

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

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);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bad value context for arguments value

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

@benjamingr
Copy link
Member

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
Copy link
Contributor Author

mscdex 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
Copy link
Contributor

refack 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
refack previously requested changes Apr 17, 2017
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

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

@mscdex
Copy link
Contributor Author

mscdex 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
Copy link
Contributor

refack 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
Copy link
Contributor Author

mscdex 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
Copy link
Contributor

refack 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
Copy link
Contributor

refack 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
Copy link
Contributor

refack 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 pushed a commit that referenced this pull request May 2, 2017
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 3, 2017
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 3, 2017
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 3, 2017
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 3, 2017
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 3, 2017
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 3, 2017
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 3, 2017
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 3, 2017
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 3, 2017
PR-URL: #12456
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@gibfahn
Copy link
Member

gibfahn commented May 15, 2017

@mscdex should this be backported to v6.x?

@mscdex
Copy link
Contributor Author

mscdex commented May 15, 2017

@gibfahn Sure.

@gibfahn gibfahn added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels May 15, 2017
@jasnell jasnell mentioned this pull request May 11, 2017
@MylesBorins MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. lib / src Issues and PRs related to general changes in the lib or src directory. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants