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

Enable FastBoot (again) #1832

Closed
wants to merge 9 commits into from
Closed

Conversation

kzys
Copy link
Contributor

@kzys kzys commented Sep 8, 2019

Compared to #1715, this branch

  • Runs 2 processes from Procfile, instead of using node-foreman.
  • Makes the number of child processes cluster creates configurable, and the default value is one.

The memory consumption is at most ~197MB on my Heroku environment. Of course the environment is having only a few requests from my browser though. Would it be acceptable?

Untitled

@rust-highfive
Copy link

r? @sgrif

(rust_highfive has picked a reviewer for you, use r? to override)

@kzys kzys mentioned this pull request Sep 8, 2019
bors added a commit that referenced this pull request Sep 8, 2019
Fix ESLint issues

During #1832, I've realized that the indentation on `app/mixins/rl-dropdown-component.js` is off.
@kzys kzys force-pushed the enable-fastboot-2 branch 3 times, most recently from ad72033 to c6df1b2 Compare September 8, 2019 14:24
@kzys
Copy link
Contributor Author

kzys commented Sep 8, 2019

At most ~299MB

Untitled 2

@kzys
Copy link
Contributor Author

kzys commented Sep 8, 2019

Okay. The last commit (ebff9fe) changed the memory consumption a lot.

Untitled 2

@bors
Copy link
Contributor

bors commented Oct 3, 2019

☔ The latest upstream changes (presumably #1837) made this pull request unmergeable. Please resolve the merge conflicts.

@kzys
Copy link
Contributor Author

kzys commented Oct 5, 2019

I will update the branch once #1853 got merged, since this branch probably will conflict with that.

@kzys kzys force-pushed the enable-fastboot-2 branch from c9b0204 to 3e89367 Compare October 24, 2019 01:53
@jtgeibel
Copy link
Member

jtgeibel commented Nov 7, 2019

I think it would be beneficial to break out enabling FastBoot (in the Procfile, nginx configuration, and app-initialized file handling) into a separate PR. That way we could land the other fixes in this PR ASAP, and then have a smaller PR that is easier to enable and revert if necessary.

It would be ideal if we could dynamically enable/disable FastBoot via an environment variable, similar to how hyper can be selected at boot time with the USE_HYPER environment variable. However, that seems tricky in this case, because we would need to dynamically configure nginx before starting it.

So I don't think we need to try to make this a dynamic configuation at this time, but I think it would make sense to break out the diff that actually enables FastBoot into its own PR and land the rest of these changes now.

@kzys
Copy link
Contributor Author

kzys commented Nov 7, 2019

I think it is possible to implement the dynamic configuration, since the Nginx Buildpack is using ERB for templating.

Anyway, I've created #1886 which contains Fastboot-related changes, except for actually enabling Fastboot. Please take a look!

@bors
Copy link
Contributor

bors commented Nov 8, 2019

☔ The latest upstream changes (presumably #1879) made this pull request unmergeable. Please resolve the merge conflicts.

bors added a commit that referenced this pull request Nov 11, 2019
Preparation for Fastboot (1/3)

This PR contains Fastboot-related fixes (from #1832) except for actually enabling Fastboot.

After merging this change, I'm going to submit a PR that updates package.json to include Fastboot in the dependencies.

Then I will submit a PR that enables Fastboot, based on USE_FASTBOOT env variable.
@kzys
Copy link
Contributor Author

kzys commented Nov 12, 2019

Since we already have #1811 for tracking. I'm going to close this PR. We may have discussions on the tracking issue.

@kzys kzys closed this Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants