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

Add program cache for executor.py #8744

Merged

Conversation

jacquesqiao
Copy link
Member

@jacquesqiao jacquesqiao commented Mar 5, 2018

related issue: #8729

return_numpy=True,
use_program_cache=False):
"""
:param program: the program that need to run
Copy link
Contributor

@panyx0718 panyx0718 Mar 6, 2018

Choose a reason for hiding this comment

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

This is core API. I would suggest more comments. For example, does the program run from begin to end, or does it just run the nodes in the fetch dependency.

Copy link
Contributor

@panyx0718 panyx0718 Mar 6, 2018

Choose a reason for hiding this comment

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

Even better, add a small python example of using Executor.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

:param program: the program that need to run
:param feed: feed variable list
:param fetch_list: fetch variable list
:param feed_var_name: feed_var_name default to 'feed'
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a bit more comment here? I'm confused about the difference between "fetch_list" and "fetch_var_name". And the meaning of default 'feed'/'fetch'.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -225,7 +226,19 @@ def run(self,
feed_var_name='feed',
fetch_var_name='fetch',
scope=None,
return_numpy=True):
return_numpy=True,
use_program_cache=False):
Copy link
Contributor

@panyx0718 panyx0718 Mar 6, 2018

Choose a reason for hiding this comment

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

I would remove this 'option' and use 'True' by default. Then as a follow up, auto-detect the program change (preferred) and auto invalidate cache.

Copy link
Member Author

Choose a reason for hiding this comment

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

currently, the cache cannot check if the program has changed, so user need to set this flag if they are sure the program has not changed, so we have to set this to false by default.

:param return_numpy: convert the fetched tensor to numpy
:param use_program_cache: set use_program_cache to true if program not changed compare to the last step.
:return:
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

more return comment?


for op in global_block.ops:
if use_program_cache:
self.program_caches[program_cache_key] = program_cache
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe limit the number cached here? Save memory if the program change often.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will drop cache of program_cache_key when use_program_cache is False now.

@dzhwinter
Copy link
Contributor

BTW, in fact, our feed, fetch operator does nothing, we use fetch_feed_method of pybind instead. Is there any profiling result about these two operators?
Maybe we should delete the feed, fetch ASAP.

panyx0718
panyx0718 previously approved these changes Mar 6, 2018
Copy link
Contributor

@panyx0718 panyx0718 left a comment

Choose a reason for hiding this comment

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

In the future, we want to auto-detect program change and refresh the cache. Currently, user explicitly set to enable cache

@@ -177,6 +193,7 @@ def __init__(self, places):
# TODO(dzhwinter) : only use the first place
self.executor = core.Executor(act_places[0])
self.places = places
self.program_caches = dict()
Copy link
Contributor

Choose a reason for hiding this comment

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

self._program_caches?

name=feed_var_name,
type=core.VarDesc.VarType.FEED_MINIBATCH,
persistable=True)
self.program_caches.pop(program_cache_key, None)
Copy link
Contributor

Choose a reason for hiding this comment

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

why pop here?

program_cache = program.clone()

if use_program_cache:
self.program_caches[program_cache_key] = program_cache
Copy link
Contributor

Choose a reason for hiding this comment

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

Leave a TODO here to avoid caching too many programs?

@jacquesqiao jacquesqiao merged commit 767acc6 into PaddlePaddle:develop Mar 6, 2018
@reyoung reyoung added this to Doing in Performance Tuning Mar 8, 2018
@reyoung reyoung moved this from Doing to Done in Performance Tuning Mar 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants