-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Avoid allocations in Ability class. #9915
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
Avoid allocations in Ability class. #9915
Conversation
It won't change anything after they are first invoke, so add method cache to avoid allocations and avoid GC. Benchmarks: ``` Calculating ------------------------------------- project_guest_rules without method cache 79.352k i/100ms project_guest_rules with method cache 93.634k i/100ms ------------------------------------------------- project_guest_rules without method cache 2.865M (±32.5%) i/s - 11.982M project_guest_rules with method cache 4.419M (± 7.4%) i/s - 22.004M Comparison: project_guest_rules with method cache: 4418908.0 i/s project_guest_rules without method cache: 2864514.0 i/s - 1.54x slower Calculating ------------------------------------- project_report_rules without method cache 53.126k i/100ms project_report_rules with method cache 97.473k i/100ms ------------------------------------------------- project_report_rules without method cache 1.093M (±36.5%) i/s - 4.675M project_report_rules with method cache 4.420M (± 7.2%) i/s - 22.029M Comparison: project_report_rules with method cache: 4420054.3 i/s project_report_rules without method cache: 1092509.6 i/s - 4.05x slower ``` https://gist.github.com/huacnlee/b04788ae6df42fe769e4
Awesome. @yorickpeterse @randx Can you take a look? |
Since all these methods return constant data, can we not just use a constant instead (and return said constant from the method)? |
@yorickpeterse That no difference but it will make code little complex and hard to read, method cache is simple. class Ability
class << self
PROJECT_GUEST_RULES = [
:read_project,
:read_wiki,
:read_issue,
:read_label,
:read_milestone,
:read_project_snippet,
:read_project_member,
:read_merge_request,
:read_note,
:read_build,
:create_project,
:create_issue,
:create_note
]
...
def project_guest_rules
PROJECT_GUEST_RULES
end
end
end VS class Ability
class << self
def project_guest_rules
@project_guest_rules ||= [
:read_project,
:read_wiki,
:read_issue,
:read_label,
:read_milestone,
:read_project_snippet,
:read_project_member,
:read_merge_request,
:read_note,
:read_build,
:create_project,
:create_issue,
:create_note
]
end
end
end |
Fair enough, I don't see any problems with using instance variables instead. |
Looks good. We will merge it a bit later (after 8.3.0.rc2) |
@randx You looks like forgot this PR, version 8.3.0 have released |
@huacnlee good reminder. Lets merge it |
Avoid allocations in Ability class.
😄 |
It won't change anything after they are first invoke, so add method cache to avoid allocations and avoid GC.
Benchmarks:
Benchark case:
https://gist.github.com/huacnlee/b04788ae6df42fe769e4