Skip to content

chore(functions): add lazy global var details #1302

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

Closed
wants to merge 1 commit into from
Closed

Conversation

ace-n
Copy link
Contributor

@ace-n ace-n commented Mar 11, 2021

Note: for the "lazy globals" section, I plan to highlight the comment I'm adding here within the sample.

Do not merge until we figure out how to handle preloading (see #1304).

@ace-n ace-n requested review from bshaffer and grayside March 11, 2021 04:36
@ace-n ace-n requested a review from a team as a code owner March 11, 2021 04:36
@product-auto-label product-auto-label bot added the api: cloudfunctions Issues related to the Cloud Run functions API. label Mar 11, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Mar 11, 2021
@product-auto-label product-auto-label bot added the samples Issues that are directly related to samples. label Mar 11, 2021
@snippet-bot
Copy link

snippet-bot bot commented Mar 11, 2021

Here is the summary of changes.

You are about to add 1 region tag.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

Comment on lines +47 to +49
// invocations, there is no benefit to "greedily" initializing them
// in global scope. Thus, we ALWAYS initialize them "lazily" within
// the function itself
Copy link
Contributor

@grayside grayside Mar 12, 2021

Choose a reason for hiding this comment

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

This guidance isn't quite right. The problem is not the lack of persisting global variables in this case, if that were the case the code below would work. The problem is that when the container starts, PHP code doesn't execute until a request comes in that loads the file.

Moving forward, we should keep in mind the larger goal of these samples: it's not about working with global variables, it's about using the language facility available to balance the amount of work done on cold start vs. function execution.

Future: We should research https://www.php.net/manual/en/opcache.preloading.php as a suggestion of this sample or a default configuration of the FF. In theory, if we set preload to the index.php file, we do get globals that can persist across requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#1304 addresses preloading. @grayside we should discuss there how we want to approach this.

(If we do decide to use preloading, then we can incorporate it into this sample.)

@@ -26,6 +26,7 @@ function _lightComputation(): int
}

// [START functions_tips_scopes]
// [START functions_tips_lazy_globals]
Copy link
Contributor

Choose a reason for hiding this comment

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

This sample is shown in the docs in adjacent sections of the same doc, I don't think it makes a lot of sense to reuse across both. I think either we don't need this snippet or it needs to be separate for clarity.

@ace-n ace-n added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 17, 2021
@ace-n
Copy link
Contributor Author

ace-n commented Apr 2, 2021

@grayside and I decided to skip this sample, given that it doesn't make sense for PHP.

(It's other-language counterparts aren't used in the docs either, so they should probably be deleted.)

@ace-n ace-n closed this Apr 2, 2021
@ace-n ace-n deleted the ace-n-patch-2 branch April 2, 2021 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: cloudfunctions Issues related to the Cloud Run functions API. cla: yes This human has signed the Contributor License Agreement. do not merge Indicates a pull request not ready for merge, due to either quality or timing. samples Issues that are directly related to samples.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants