Skip to content

Add caching to guarded_import to boost performance#163

Open
jan-jockusch wants to merge 2 commits intomasterfrom
cache-guarded-import
Open

Add caching to guarded_import to boost performance#163
jan-jockusch wants to merge 2 commits intomasterfrom
cache-guarded-import

Conversation

@jan-jockusch
Copy link
Copy Markdown

@jan-jockusch jan-jockusch commented Apr 28, 2026

Using "import" in Python Scripts triggers the execution of guarded_import each time. This performs a security test and then the import itself. The import is fast because the Python interpreter caches this. The security test is costly, and always yields the same result. This patch inserts a cache to skip this test on repeated executions, speeding up the Python Scripts.

  • I signed and returned the Zope Contributor Agreement, and received and accepted an invitation to join a team in the zopefoundation GitHub organization.
  • I verified there aren't any other open pull requests for the same change.
  • I followed the guidelines in Developer guidelines.
  • I successfully ran code quality checks on my changes locally.
  • I successfully ran tests on my changes locally.
  • If needed, I added new tests for my changes.
  • If needed, I added documentation for my changes.
  • I included a change log entry in my commits.

If your pull request closes an open issue, include the exact text below, immediately followed by the issue number. When your pull request gets merged, then that issue will close automatically.

Closes #

Using "import" in Python Scripts triggers the execution of
guarded_import each time. This performs a security test and
then the import itself. The import is fast because the Python
interpreter caches this. The security test is costly, and always
yields the same result. This patch inserts a cache to skip this
test on repeated executions, speeding up the Python Scripts.
@jan-jockusch
Copy link
Copy Markdown
Author

I cannot push to my branch. Therefore, the changelog is still missing.

@dataflake
Copy link
Copy Markdown
Member

Please try the push again, I just deleted a broken branch protection rule that wantd to cover all possible branches, which makes no sense.

@jan-jockusch
Copy link
Copy Markdown
Author

Please try the push again, I just deleted a broken branch protection rule that wantd to cover all possible branches, which makes no sense.

Yes! Push has worked. Thank you!

Copy link
Copy Markdown
Contributor

@perrinjerome perrinjerome left a comment

Choose a reason for hiding this comment

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

This seems a good idea, but for correctness I believe this cache should be invalidated when allow_module is called.

@jan-jockusch
Copy link
Copy Markdown
Author

I'll look into adding clearing the cache on allow_module as suggested.

For motivation: This shaves off 4% of time spent in our scenario, with many scripts in Data.fs utilizing import. YMMV, but I believe this is the near the max value achievable in terms of performance gains, since we use import rather voraciously.

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.

3 participants