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

Resolve shell env API performance issue for single env approach #238488

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

anthonykim1
Copy link
Contributor

@anthonykim1 anthonykim1 commented Jan 22, 2025

Potential root of the performance cause:

  1. Using diff approach where we check for change the environment variables, and only firing an event if there is a difference, should resolve the problem
  2. O(n) - iterating through each environment variable in the process of fetching them (inside shell such as bash) is what is slow
  3. Print statements in the above O(n) is what is causing the slowness (shell side: e.g. bash side)
  4. O(n) - iterating through each environment variable when trying to calculate the diff is what is slow (in the shellEnvDetectionCapability)

Edit: d3ef701 brought back instant bash prompts. I think it was combination of printenv with compgen and tons of printf statements really slowing things down. So Mainly Scenario number 3 was correct

@anthonykim1 anthonykim1 self-assigned this Jan 22, 2025
@anthonykim1
Copy link
Contributor Author

anthonykim1 commented Jan 22, 2025

https://github.com/user-attachments/assets/57167cc9-fd5b-424b-b302-83542592365d
@Tyriar This is for bash, but I think I brought back the perf to normal

@anthonykim1 anthonykim1 requested a review from Tyriar January 26, 2025 22:40
@kieferrm kieferrm mentioned this pull request Jan 27, 2025
62 tasks
@anthonykim1
Copy link
Contributor Author

TODO: Should address #238338 (comment) if we were to enable shell env for bash again since it is critical.

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.

4 participants