Skip to content
This repository was archived by the owner on Jul 8, 2022. It is now read-only.

Conversation

@mliszcz
Copy link
Collaborator

@mliszcz mliszcz commented Jun 12, 2019

To slightly improve remove_attribute performance when multiple writable attributes are configured (fixes #556).

t-b
t-b previously approved these changes Jul 30, 2019
Copy link
Collaborator

@t-b t-b left a comment

Choose a reason for hiding this comment

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

Looks good.

Two minor comments:

  • If we can rely on get_attr_ind_by_name doing the transform->lower we can ditch it here.
  • I'm a big fan of reducing indentation and returning early. So I would write it as
if (write_type != Tango::READ_WITH_WRITE && write_type != Tango::READ_WRITE)
  {
    return;
  }

but that is just taste I guess.

@mliszcz mliszcz force-pushed the fix-556-remove-attr-performance branch from 7a0c4f1 to 13d04b0 Compare November 26, 2019 12:29
@mliszcz
Copy link
Collaborator Author

mliszcz commented Nov 26, 2019

Thanks @t-b. I've changed the PR as you suggested. @bourtemb could you please check as well?

@t-b t-b requested a review from Ingvord December 12, 2019 12:25
@t-b t-b added the Candidate For Backport Requires a backport to the release branches label Apr 24, 2020
Copy link
Member

@bourtemb bourtemb left a comment

Choose a reason for hiding this comment

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

Thanks @mliszcz
Just a comment on the origin field in the exceptions thrown, but this was like that before this PR.
I think we could change that.
Except that, this PR looks good to me!

@t-b t-b self-requested a review July 7, 2020 12:03
@mliszcz mliszcz merged commit 3715320 into tango-controls:tango-9-lts Jul 7, 2020
@mliszcz mliszcz deleted the fix-556-remove-attr-performance branch July 7, 2020 12:16
@t-b
Copy link
Collaborator

t-b commented Jul 9, 2020

@mliszcz Will you do a backports PR?

@mliszcz
Copy link
Collaborator Author

mliszcz commented Jul 9, 2020

@t-b sure, I can backport if you want. I don't know what is the policy for backports - do we backport everything that can be backported or just critical bugfixes? Can I assume that I should backport everything that is tagged with "Candidate for backport" maybe?

@t-b
Copy link
Collaborator

t-b commented Jul 9, 2020

@mliszcz Well I thought you added the "Candidate for backport" tag, but that was me. From my understanding "Candidate for backport" means that this should get backported. But maybe we need a different name then. @bourtemb Thoughts?

@bourtemb
Copy link
Member

bourtemb commented Jul 9, 2020

From my understanding "Candidate for backport" means that this should get backported.

I think we can take this convention indeed but we might need to discuss before to put this tag on a PR to decide whether it is worth being backported.

In the case of this PR, I would say if it is easy to backport, we can do it but it does not look like a critical bug fix to me, so I would personally not put a high priority on this one.

@mliszcz
Copy link
Collaborator Author

mliszcz commented Jul 9, 2020

I'll send a backport PR. It was waiting for a review for more than a year without conflicts. I think I just need to cherry-pick relevant commits and do not expect any conflicts in 9.3-backports.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Candidate For Backport Requires a backport to the release branches Ready For Merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Poor performance when removing dynamic attributes

3 participants