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

[dialog] stats incremented twice #1566

Merged
merged 1 commit into from Jun 18, 2018

Conversation

jchavanton
Copy link
Member

Pre-Submission Checklist

  • Commit message has the format required by CONTRIBUTING guide
  • Commits are split per component (core, individual modules, libs, utils, ...)
  • Each component has a single commit (if not, squash them into one commit)
  • No commits to README files for modules (changes must be done to docbook files
    in doc/ subfolder, the README file is autogenerated)

Type Of Change

  • Small bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds new functionality)
  • Breaking change (fix or feature that would change existing functionality)

Checklist:

  • PR should be backported to stable branches
  • Tested changes locally
  • Related to issue #XXXX (replace XXXX with an open issue number)

Description

As you can see looking at the modifications stats counters where increment twice.

Hi @oej, If you find a few minutes you could review this MR.
Not sure if you remember, this was introduced in a bug fix (see below)
I kept some of your modifications, they seem more strait forward anyway.

commit b0cd09d2b451005a736396e6f38eac139ec31301
Author: Olle E. Johansson <oej@edvina.net>
Date:   Wed May 11 21:59:44 2016 +0200

    dialog Make sure statistics are updated when initializing from database
    
    Issue #424

@jchavanton
Copy link
Member Author

update_stat is optimized inline static void update_stat(stat_var* v, int n) so the extra code to call if_update_stat only once was indeed not needed

@henningw henningw self-assigned this Jun 17, 2018
@henningw
Copy link
Contributor

Thank you for the patch. It seems that there was a change in the functionality regarding the stats counter implementation.
I don't understand your comment regarding the update_stats method though. In your patch the if_update_stat macro is called, which then calls update_stat.?

@jchavanton
Copy link
Member Author

jchavanton commented Jun 18, 2018

Hi Henning, in the end we were calling if_update_stat twice.
This code is only called when retrieving dialog from the DB.

Yes my comment was about removing the optimized version and keeping the simpler one, if_update_stat is is a macro calling several inline functions.
The optimized way was to using a counter in order to call if_update_stat only once.
Then as second call to if_update_stat was added in b0cd09 probably because the optimization with the counter was obfuscating things slightly.

I think the optimization is not needed the cost per dialog seems very low.
But we could still select to keep the optimized version if we want, knowing that since 2016 both version were used anyway.

Thanks for the review

@henningw henningw merged commit 9317175 into kamailio:master Jun 18, 2018
@henningw
Copy link
Contributor

Thank you

@jchavanton jchavanton deleted the invalid_dialog_count branch June 25, 2018 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants