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

[swssconfig] Optimize performance of swssconfig #2336

Merged
merged 4 commits into from
Jun 24, 2022

Conversation

Junchao-Mellanox
Copy link
Collaborator

What I did

Optimize swssconfig:

  • Use unix socket
  • Cache producer table to avoid create it for same table name

Why I did it

We found that generating large scale static routes via swssconfig is very slow.

How I verified it

After the optimization, generating 100K routes via swssconfig take 2 seconds, however, before the optimization it takes > 60 seconds.

Details if related

@lgtm-com
Copy link

lgtm-com bot commented Jun 17, 2022

This pull request introduces 1 alert when merging 556d703 into 700492f - view on LGTM.com

new alerts:

  • 1 for Short global name

SWSS_LOG_ERROR("Invalid operation: %s\n", kfvOp(db_item).c_str());
return false;
}
}

flush_db_data();
Copy link
Contributor

@qiluo-msft qiluo-msft Jun 17, 2022

Choose a reason for hiding this comment

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

flush_db_data

I see 3 flush_db_data(). Is it the same just calling once outside? #Closed

Copy link
Collaborator Author

@Junchao-Mellanox Junchao-Mellanox Jun 17, 2022

Choose a reason for hiding this comment

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

Here, I want to keep the logic as same as before. Before the change, all data will be put into redis DB until a failure occurs. So, here I call flush_db_data() at 3 places (2 return false, 1 return true) to make sure the data will be flushed into redis even if a failure happens.

Copy link
Contributor

Choose a reason for hiding this comment

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

Understand your point. The pattern here is a python finally like syntax to achieve cleanup if out of scope. C++ has some best practice like RAII to achieve this.

Actually in this case, if you limit table_map scope to this function, not as a global function. The dtor will automatically flush all pipelines.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suppose I understand your point. RedisPipleLine will flush all data in its dtor, so if pipeline is defined in this function, it will automatically do flush. I will make this change.

@qiluo-msft qiluo-msft requested a review from liuh-80 June 17, 2022 05:54
@lgtm-com
Copy link

lgtm-com bot commented Jun 17, 2022

This pull request introduces 1 alert when merging ba88348 into 700492f - view on LGTM.com

new alerts:

  • 1 for Short global name

@qiluo-msft
Copy link
Contributor

Could you fix LGTM alert?

@Junchao-Mellanox
Copy link
Collaborator Author

/azpw run Azure.sonic-swss

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Junchao-Mellanox
Copy link
Collaborator Author

It seems the checker is running a test case which is newer than my base code, I need to do a merge now.

@Junchao-Mellanox
Copy link
Collaborator Author

/azpw run Azure.sonic-swss

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liat-grozovik
Copy link
Collaborator

/azp run Azure.sonic-swss

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liat-grozovik liat-grozovik merged commit 37349cf into sonic-net:master Jun 24, 2022
yxieca pushed a commit that referenced this pull request Jun 25, 2022
- What I did
Optimize swssconfig:
1. Use unix socket
2. Cache producer table to avoid create it for same table name

- Why I did it
We found that generating large scale static routes via swssconfig is very slow.

- How I verified it
After the optimization, generating 100K routes via swssconfig take 2 seconds, however, before the optimization it takes > 60 seconds.
jimmyzhai added a commit to sonic-net/sonic-buildimage that referenced this pull request Jun 27, 2022
2022-06-24 93af69c: [PFC_WD] Avoid applying ZeroBuffer Profiles to ingress PG when a PFC storm is detected (sonic-net/sonic-swss#2304)
2022-06-24 37349cf: [swssconfig] Optimize performance of swssconfig (sonic-net/sonic-swss#2336)
2022-06-24 84e9b07: [fdborch] fix heap-use-after-free in clearFdbEntry() (sonic-net/sonic-swss#2353)
2022-06-24 1b8bd94: Create ACL table fails due to incorrect check for supported ACL actions #11235 (sonic-net/sonic-swss#2351)
2022-06-24 1ed0b4b: [macsec] Refactor the logic of macsec name map (sonic-net/sonic-swss#2348)
2022-06-23 f88f992: [mock_tests] Add Sflow Orch UTs (sonic-net/sonic-swss#2295)
2022-06-23 ec57bf1: [macsec] Update macsec flex counter (sonic-net/sonic-swss#2338)
2022-06-22 6e0fc85: [ACL] Support stage particular match fields (sonic-net/sonic-swss#2341)
2022-06-22 efb4530: [orchagent, DTel]: report session support to set user vrf (sonic-net/sonic-swss#2326)
2022-06-22 d82874d: Fix for "orchagent crashed when trying to delete fdb static entry with swssconfig #11046" (sonic-net/sonic-swss#2332)
2022-06-22 0c789e6: Fix qos map test in vs test (sonic-net/sonic-swss#2343)
2022-06-17 1bb5070: Enhance mock test for dynamic buffer manager for port removing and qos reload flows (sonic-net/sonic-swss#2262)
2022-06-16 700492f: [aclorch] Fix and simplify DTel watchlist tables and entries (sonic-net/sonic-swss#2155)
preetham-singh pushed a commit to preetham-singh/sonic-swss that referenced this pull request Aug 6, 2022
- What I did
Optimize swssconfig:
1. Use unix socket
2. Cache producer table to avoid create it for same table name

- Why I did it
We found that generating large scale static routes via swssconfig is very slow.

- How I verified it
After the optimization, generating 100K routes via swssconfig take 2 seconds, however, before the optimization it takes > 60 seconds.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants