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

tools: allow explicitly opting in to low-fi status display #9341

Merged
merged 9 commits into from Dec 13, 2017

Conversation

@glasser
Copy link
Member

@glasser glasser commented Nov 10, 2017

I think this was originally designed for Emacs shell but at some point Emacs or
Node improved so that Node finds columns for Emacs shell and switches to the
full display.

Switching to the status-only display leads to massive speedups when doing builds
in my Emacs shell.

I think this was originally designed for Emacs shell but at some point Emacs or
Node improved so that Node finds columns for Emacs shell and switches to the
full display.

Switching to the status-only display leads to massive speedups when doing builds
in my Emacs shell.
@glasser glasser requested a review from abernix Nov 10, 2017
@glasser
Copy link
Member Author

@glasser glasser commented Nov 10, 2017

Hmm. @abernix suggested trying METEOR_HEADLESS and it seems to work about as well. That said, I do think that ProgressDisplayStatus is effectively dead code, which isn't great.

@abernix
Copy link
Member

@abernix abernix commented Nov 15, 2017

Thank you for looking at this! Another hack week success story!

We currently have METEOR_PRETTY_OUTPUT, METEOR_HEADLESS and EMACS, all of which are supposed to change the output method in some way, so I'm a bit hesitant to add another at the moment, especially if METEOR_HEADLESS resolves the problem. I hope you understand my hesitation to further complicate this, though I completely respect your point about the dead code. We will fix that, somehow! (It's not clear to me when _stream.isTTY && !_stream.columns was important to fix—Docker, perhaps?)

I'd like to make this process better, in general but especially for Emacs usage, right now, without the need to opt in. While I had previously thought that EMACS was an automatically detected option, I believe it's actually just a manual environment variable, especially since it has to be set to exactly t. As you indicated, INSIDE_EMACS is actually what's automatically set in an Emacs environment.

Would you be able to check how performance is if you:

  • Unset METEOR_HEADLESS, if you're still using it.
  • Set EMACS specifically to t. 🤷‍♂️

?

Depending on the outcome, perhaps we can replace the manual EMACS with an automatically detected INSIDE_EMACS, or even use it in place of this new METEOR_PROGRESS_STATUS_ONLY variable.

@glasser
Copy link
Member Author

@glasser glasser commented Nov 15, 2017

I don't think Meteor looks at EMACS for anything other than readline styling and tweaking some stdin properties. Seems ignored by the progress stuff.

@glasser
Copy link
Member Author

@glasser glasser commented Nov 15, 2017

Apparently INSIDE_EMACS was introduced as a replacement for EMACS in Emacs 22 in 2007, and EMACS was dropped in Emacs 25 in 2016.

@benjamn benjamn self-requested a review Nov 22, 2017
abernix added 6 commits Dec 12, 2017
This adds a memoized helper function for detecting Emacs throughout the
CLI tool and automatically resorts to "ProgressDisplayStatus" mode when
Emacs is enabled.

In my tests with both Emacs 22 and 27, this nearly doubled performance
when using Meteor from within the Emacs shell (i.e. `M-x shell`).
To avoid any unintended inheritance of non-"own" properties.
@@ -71,7 +71,6 @@ build_machine_environment: &build_machine_environment

# These, mostly overlapping, flags ensure that CircleCI is as pretty as
# possible for a non-interactive environment. See also: --headless.
EMACS: t

This comment has been minimized.

@abernix

abernix Dec 12, 2017
Member

With METEOR_HEADLESS enabled, we're not really gaining anything by using this setting in CircleCI.

// clearLine() and cursorTo(...).
// It's important that we only enter status message mode
// if this._pretty, so that we don't start displaying
// status messages too soon.

This comment has been minimized.

@abernix

abernix Dec 12, 2017
Member

I removed this as it seemed a bit verbose, but in hindsight, I guess this comment was referring to the importance of this being later on in the conditional chain (after the else if on 1180). Happy to add it back if its removal detracts from the readability.

This comment has been minimized.

@hwillson

hwillson Dec 12, 2017
Member

I'm okay with leaving it out! 👍

@abernix abernix requested review from benjamn and hwillson and removed request for benjamn Dec 12, 2017
Copy link
Member

@hwillson hwillson left a comment

Looks great @abernix! And goodbye more _.! 🎉

@@ -16,6 +16,7 @@ var catalog = require('../packaging/catalog/catalog.js');
var buildmessage = require('../utils/buildmessage.js');
var httpHelpers = require('../utils/http-helpers.js');
const archinfo = require('../utils/archinfo.js');
import { isEmacs } from "../utils/utils.js";

This comment has been minimized.

@hwillson

hwillson Dec 12, 2017
Member

We better add a isVi function in utils.js just to be safe ... 🔥😱

This comment has been minimized.

@abernix

abernix Dec 12, 2017
Member

For the record, the speed is almost/exactly as fast as a native terminal session when running vi/vim split-screen with a terminal in tmux or screen. 😉

// clearLine() and cursorTo(...).
// It's important that we only enter status message mode
// if this._pretty, so that we don't start displaying
// status messages too soon.

This comment has been minimized.

@hwillson

hwillson Dec 12, 2017
Member

I'm okay with leaving it out! 👍

Copy link
Member Author

@glasser glasser left a comment

LGTM! Feel free to take it from here.

@abernix abernix added this to the Release 1.6.1 milestone Dec 13, 2017
abernix added 2 commits Dec 13, 2017
To fix my typo from the previous commit.
@abernix abernix merged commit 9b056f5 into devel Dec 13, 2017
1 check passed
1 check passed
CLA Author has signed the Meteor CLA.
Details
@abernix abernix deleted the glasser/progress-status-only branch Dec 13, 2017
@trusktr

This comment has been minimized.

Copy link
Contributor

@trusktr trusktr commented on d35b1e2 Dec 14, 2017

Can you explain this a little more? You mean you want objects without any of Object.prototype properties? Why for? Does it improve performance?

This comment has been minimized.

Copy link
Contributor

@trusktr trusktr replied Dec 14, 2017

@Slava

This comment has been minimized.

Copy link
Member

@Slava Slava commented on 7268fb8 Jul 15, 2019

Interesting, I wonder why accessing process.env would be so much slower than seemingly an equivalent variable? As far as I know process.env is copied once in the beginning of node init and doesn't change/doesn't make any additional look-ups 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.