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

Keep attribute order in bulk mode #1659

Merged
merged 2 commits into from
Mar 8, 2021
Merged

Keep attribute order in bulk mode #1659

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ab95ce7
Keep attribute order in bulker mode
Mar 3, 2021
5f219aa
Add unit test
Mar 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 16 additions & 29 deletions orchagent/bulker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,10 +208,10 @@ class EntityBulker
if (found_setting != setting_entries.end())
{
// Mark old one as done
auto& attrmap = found_setting->second;
for (auto& attr: attrmap)
auto& attrs = found_setting->second;
for (auto& attr: attrs)
{
*attr.second.second = SAI_STATUS_SUCCESS;
*attr.second = SAI_STATUS_SUCCESS;
}
// Erase old one
setting_entries.erase(found_setting);
Expand Down Expand Up @@ -252,27 +252,15 @@ class EntityBulker
if (!attr) throw std::invalid_argument("attr is null");

// Insert or find the key (entry)
auto& attrmap = setting_entries.emplace(std::piecewise_construct,
auto& attrs = setting_entries.emplace(std::piecewise_construct,
std::forward_as_tuple(*entry),
std::forward_as_tuple()
).first->second;

// Insert or find the key (attr)
auto rc = attrmap.emplace(std::piecewise_construct,
std::forward_as_tuple(attr->id),
std::forward_as_tuple());
bool inserted = rc.second;
auto it = rc.first;

// If inserted new key, assign the attr
// If found existing key, overwrite the old attr
Copy link
Contributor

@qiluo-msft qiluo-msft Mar 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If found existing key, overwrite the old attr [](start = 11, length = 45)

One advantage or map structure is possible to merge setting operation on the same entry and same attribute. Is it still required? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it does not seem very necessary to merge the operations for the same attribute and the same entry because of the following reasons:

  1. The same entry should not show up more than once in one bulk.
  2. For each route entry, there are two attributes that might be set in bulk mode: PACKET_ACTION and NEXT_HOP_ID, each gets to set only once in the routeorch logic.
  3. For other types of entries (not supported in bulk yet), the to_sync logic should already merge the same attribute for the same entry. It does not seem necessary to do it again in bulk.

it->second.first = *attr;
if (!inserted)
{
// If found existing key, mark old status as success
*it->second.second = SAI_STATUS_SUCCESS;
}
it->second.second = object_status;
// Insert attr
attrs.emplace_back(std::piecewise_construct,
std::forward_as_tuple(*attr),
std::forward_as_tuple(object_status));
*object_status = SAI_STATUS_NOT_EXECUTED;
}

Expand Down Expand Up @@ -351,19 +339,21 @@ class EntityBulker
{
std::vector<Te> rs;
std::vector<sai_attribute_t> ts;
std::vector<sai_status_t*> status_vector;

for (auto const& i: setting_entries)
{
auto const& entry = i.first;
auto const& attrmap = i.second;
for (auto const& ia: attrmap)
auto const& attrs = i.second;
for (auto const& ia: attrs)
{
auto const& attr = ia.second.first;
sai_status_t *object_status = ia.second.second;
auto const& attr = ia.first;
sai_status_t *object_status = ia.second;
if (*object_status == SAI_STATUS_NOT_EXECUTED)
{
rs.push_back(entry);
ts.push_back(attr);
status_vector.push_back(object_status);
}
}
}
Expand All @@ -375,9 +365,7 @@ class EntityBulker

for (size_t ir = 0; ir < count; ir++)
{
auto& entry = rs[ir];
auto& attr_id = ts[ir].id;
sai_status_t *object_status = setting_entries[entry][attr_id].second;
sai_status_t *object_status = status_vector[ir];
if (object_status)
{
SWSS_LOG_INFO("EntityBulker.flush setting_entries status[%zu]=%d(0x%8p)\n", ir, statuses[ir], object_status);
Expand Down Expand Up @@ -426,8 +414,7 @@ class EntityBulker

std::unordered_map< // A map of
Te, // entry ->
std::unordered_map< // another map of
sai_attr_id_t, // attr_id ->
std::vector< // vector of attribute and status
std::pair<
sai_attribute_t, // (attr_value, OUT object_status)
sai_status_t *
Expand Down