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

Using the cache for function bindings and not deleting shared memory resources upon request to close #844

Merged
merged 17 commits into from
Oct 2, 2021

Conversation

gohar94
Copy link
Contributor

@gohar94 gohar94 commented Apr 28, 2021

Description

When the host sends a message to close shared memory resources, instead of deleting the shared memory resources (i.e. files in the case of Linux) just drop the handle and let the worker delete the resources.

When the host advertises the capability for using FunctionDataCache, use that even for small objects (without imposing the minimum size constraint.)

This PR is part of the work described in Azure/azure-functions-host#7310


PR information

  • The title of the PR is clear and informative.
  • There are a small number of commits, each of which has an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
  • If applicable, the PR references the bug/issue that it fixes in the description.
  • New Unit tests were added for the changes made and CI is passing.

Quality of Code and Contribution Guidelines

@gohar94 gohar94 marked this pull request as draft April 28, 2021 22:47
@gohar94 gohar94 self-assigned this Apr 28, 2021
@gohar94 gohar94 changed the title Gochaudh/caching Using the cache for function bindings and not deleting shared memory resources upon request to close Apr 28, 2021
azure_functions_worker/bindings/meta.py Outdated Show resolved Hide resolved
azure_functions_worker/bindings/meta.py Outdated Show resolved Hide resolved
@gohar94 gohar94 marked this pull request as ready for review September 14, 2021 18:26
Copy link
Member

@vrdmr vrdmr left a comment

Choose a reason for hiding this comment

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

minor comments. Also, can you please add testing for cache-enabled scenarios? to see if the same shared mem is reused or not?

@gohar94
Copy link
Contributor Author

gohar94 commented Sep 20, 2021

minor comments. Also, can you please add testing for cache-enabled scenarios? to see if the same shared mem is reused or not?

That's a very good point.
The problem right now is that the caching piece will not be enabled until one of the azure storage extension PRs is checked in. That PR will not be checked in until the azure-webjobs-SDK PR package is published to nuget.org - this will happen in a few weeks as part of some deployment cycle which I don't fully understand. Once that is checked in, then we can have a subsequent PR here for the Python worker which consumes the latest host and storage extension in the tests so that we can put the tests you mentioned. Does that sounds like a fair plan?

azure_functions_worker/dispatcher.py Outdated Show resolved Hide resolved
azure_functions_worker/bindings/meta.py Outdated Show resolved Hide resolved
azure_functions_worker/bindings/meta.py Outdated Show resolved Hide resolved
azure_functions_worker/bindings/meta.py Outdated Show resolved Hide resolved
@ghost ghost removed the needs-author feedback label Oct 1, 2021
Copy link
Member

@vrdmr vrdmr left a comment

Choose a reason for hiding this comment

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

nit changes. otherwise, LGTM. 🚢

Comment on lines 540 to 549
if self._function_data_cache_enabled:
# If the cache is enabled, let the host decide when to
# delete the resources.
# Just drop the reference from the worker.
to_delete = False
else:
# If the cache is not enabled, the worker should free
# the resources as at this point the host has read the
# memory maps and does not need them.
to_delete = True
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if self._function_data_cache_enabled:
# If the cache is enabled, let the host decide when to
# delete the resources.
# Just drop the reference from the worker.
to_delete = False
else:
# If the cache is not enabled, the worker should free
# the resources as at this point the host has read the
# memory maps and does not need them.
to_delete = True
to_delete_reference = False if self._function_data_cache_enabled else True

And please add the comment in the python docs section of this method itself.

# the resources as at this point the host has read the
# memory maps and does not need them.
to_delete = True
success = self._shmem_mgr.free_mem_map(map_name, to_delete)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
success = self._shmem_mgr.free_mem_map(map_name, to_delete)
success = self._shmem_mgr.free_mem_map(map_name, to_delete_reference)

If above suggestion is included, please include this and also check if there is any other place to_delete is mentioned.

@vrdmr vrdmr merged commit 6e3ecd4 into dev Oct 2, 2021
@vrdmr vrdmr deleted the gochaudh/caching branch October 2, 2021 01:32
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