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

Use template hgetall, because we will tune the return types of library functions #759

Merged
merged 1 commit into from
Jan 4, 2021
Merged

Use template hgetall, because we will tune the return types of library functions #759

merged 1 commit into from
Jan 4, 2021

Conversation

qiluo-msft
Copy link
Contributor

The template version of hgetall() could work with either map or unordered_map.

@kcudnik
Copy link
Collaborator

kcudnik commented Jan 3, 2021

can you point to changes in swss common which will address this?

@qiluo-msft
Copy link
Contributor Author

@kcudnik we change the returned type of hgetall by https://github.com/Azure/sonic-swss-common/pull/437/files#diff-6bae96c20cc412d0c97e2a241f0cdc8c06d1a90fc13697faf1c8e43f28d38da3R181

This PR will work fine with both old code and new code in sonic-swss-common.

@kcudnik
Copy link
Collaborator

kcudnik commented Jan 3, 2021

@kcudnik we change the returned type of hgetall by https://github.com/Azure/sonic-swss-common/pull/437/files#diff-6bae96c20cc412d0c97e2a241f0cdc8c06d1a90fc13697faf1c8e43f28d38da3R181

This PR will work fine with both old code and new code in sonic-swss-common.

why there is change from unordered_map to map? map is slower nlogn to sort items, and we dont need sort at hgetall, it will slow things down when we will have 100k entreis in db, that will matter, please reverse that to unordered_map

@qiluo-msft
Copy link
Contributor Author

Offline discussed. In this PR, sairedis does not change to map at all. It will call a template version of hgetall, which accepts a container inserter. So sairedis is still using unordered_map.
We will keep swss-common behavior as before, there will be another PR for it.

@qiluo-msft
Copy link
Contributor Author

Retest vs please

@lguohan
Copy link
Contributor

lguohan commented Jan 4, 2021

vstest has issues, https://sonic-jenkins.westus2.cloudapp.azure.com/job/vs/job/sonic-swss-build/

@lguohan
Copy link
Contributor

lguohan commented Jan 4, 2021

the four test failure are expected.

@lguohan lguohan merged commit 6385d49 into sonic-net:master Jan 4, 2021
@kcudnik kcudnik mentioned this pull request Jan 4, 2021
Merged
@kcudnik
Copy link
Collaborator

kcudnik commented Jan 4, 2021

why those specific 4 tests are allowed to fail ?

@qiluo-msft qiluo-msft deleted the qiluo/getalltype branch January 4, 2021 17:16
@lguohan
Copy link
Contributor

lguohan commented Jan 4, 2021

it should not, @daall is working on to triage the 4 failed test cases.

pettershao-ragilenetworks pushed a commit to pettershao-ragilenetworks/sonic-sairedis that referenced this pull request Nov 18, 2022
@qiluo-msft
Use template hgetall, because we will tune the return types of librar…
0cfe091 
…y functions (sonic-net#759)

The template version of hgetall() could work with either map or unordered_map.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lguohan lguohan lguohan approved these changes

@kcudnik kcudnik kcudnik 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
@qiluo-msft @kcudnik @lguohan