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.

Sign up for GitHub

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

Jump to bottom

[sonic-cfggen] optimize sonic-cfggen startup #3658

Merged
merged 4 commits into from
Oct 31, 2019
Merged

[sonic-cfggen] optimize sonic-cfggen startup #3658

merged 4 commits into from
Oct 31, 2019

Conversation

stepanblyschak
Copy link
Collaborator

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

Stepan Blyschak added 2 commits October 23, 2019 17:42
[sonic-cfggen] optimize execution time
61e7b4b 
a lot of template rendering causes switch to start longer because jinja2
needs to parse them. Introducing RedisBytecodeCache to store parsed buckets of
internal template bytecode to speedup same template rendering during start

Signed-off-by: Stepan Blyschak <stepanb@mellanox.com>
[sonic-cfggen] do lazy regexp compilation to speedup sonic-cfggen
ad264e3 
Signed-off-by: Stepan Blyschak <stepanb@mellanox.com>
@lguohan
Copy link
Collaborator

lguohan commented Oct 23, 2019

do you have a benchmark number?

@lguohan lguohan requested a review from qiluo-msft October 24, 2019 06:57
@stepanblyschak
Copy link
Collaborator Author

@lguohan
cfggen_prof.txt
Test done with lazy_re. Note, sonic-cfggen is called over 100 times during start, this saves 8 sec for LAG restoration time. Ideally I think we could rethink the way start scripts work to eliminate many sonic-cfggen invocations.
Sonic-cfggen functionality could be implemented as a library that start scripts import only once (import jinja2, natsort are expensive because many static regular expressions are compiled at import time). This I believe will give a boost for start script like in bgp docker container https://github.com/Azure/sonic-buildimage/blob/master/dockers/docker-fpm-frr/start.sh where it renders many templates.
For scripts that require only hwsku, platform, hostname there could be a module that does not import heavy packages at all, since all you need is single redis query or read from machine.conf.
Some templates are questionable if they even have to be templates at all, like https://github.com/Azure/sonic-buildimage/blob/master/dockers/docker-dhcp-relay/wait_for_intf.sh.j2. A script could do this at runtime with few redis queries.
To speedup a bit templates rendering we can to use jinja cache in redis.
What do you think?

qiluo-msft
qiluo-msft reviewed Oct 24, 2019
View reviewed changes
src/sonic-config-engine/sonic-cfggen Outdated
@@ -16,6 +16,16 @@ See usage string for detail description for arguments.
"""

from __future__ import print_function

# mokey patch re.compile to do lazy regular expression compilation.
Copy link
Collaborator

@qiluo-msft qiluo-msft Oct 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

mokey [](start = 2, length = 5)

typo #Closed

qiluo-msft
qiluo-msft reviewed Oct 24, 2019
View reviewed changes
src/sonic-config-engine/lazy_re.py Outdated

re.compile = __re_compile


Copy link
Collaborator

@qiluo-msft qiluo-msft Oct 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Several code format issues. Please check http://pep8online.com #Closed

qiluo-msft
qiluo-msft reviewed Oct 24, 2019
View reviewed changes
src/sonic-config-engine/redis_bcc.py
def __init__(self, client):
self._client = client
try:
self._client.connect(self._client.STATE_DB, retry_on=False)
Copy link
Collaborator

Choose a reason for hiding this comment

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

STATE_DB [](start = 46, length = 8)

use another non-used database?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can I use db index 8 and update swss-common/swss-sdk?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is still applicable.

In reply to: 338815799 [](ancestors = 338815799)

@qiluo-msft
Copy link
Collaborator

qiluo-msft commented Oct 24, 2019 •
edited
Loading

do you have a benchmark for RedisBytecodeCache improvement? #Closed

@lguohan
Copy link
Collaborator

lguohan commented Oct 25, 2019

@stepanblyschak , i agree with all comments. good ideas!

@lguohan
Copy link
Collaborator

lguohan commented Oct 25, 2019

please address other reviewers comments.

Stepan Blyschak added 2 commits October 28, 2019 15:59
[sonic-cfggen] address pep8 related comments
c33efd8 
Signed-off-by: Stepan Blyschak <stepanb@mellanox.com>
[sonic-cfggen] address typo comment
3a03a12 
Signed-off-by: Stepan Blyschak <stepanb@mellanox.com>
@stepanblyschak
Copy link
Collaborator Author

profiling_redis_bcc.txt

@lguohan lguohan merged commit 064689d into sonic-net:master Oct 31, 2019
@lguohan lguohan mentioned this pull request Oct 31, 2019
Closed
@lguohan lguohan mentioned this pull request Oct 11, 2020
Closed
@stepanblyschak stepanblyschak deleted the cfggen_opt branch September 23, 2022 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qiluo-msft qiluo-msft qiluo-msft left review comments

@lguohan lguohan lguohan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stepanblyschak @lguohan @qiluo-msft