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

Use hasmap to find comment data #374 #375

Closed
wants to merge 1 commit into from
Closed

Conversation

nook24
Copy link
Member

@nook24 nook24 commented Nov 24, 2021

This is my attempt to patch #374. Would be good if you could review this.
Signed-off-by: nook24 [email protected]

@sni
Copy link
Contributor

sni commented Nov 24, 2021

Now there is a comment_hashtable and a comment_hashlist. Have you checked if we need both by any chance?

@nook24
Copy link
Member Author

nook24 commented Nov 25, 2021

Tbh, I'm not sure if I understand whats happening with comment_hashlist by 100%. It is only used by comment *get_next_comment_by_host(char *host_name, comment *start). I think it is possible to get rid of it but I tried to touched as less code as possible.

How ever, by removing comment_hashlist we could also delete this:

/**************************************************
*************** HASH FUNCTIONS *******************
**************************************************/
/* dual hash function */
int hashfunc(const char *name1, const char *name2, int hashslots)
{
unsigned int i, result;
result = 0;
if (name1)
for (i = 0; i < strlen(name1); i++)
result += name1[i];
if (name2)
for (i = 0; i < strlen(name2); i++)
result += name2[i];
result = result % hashslots;
return result;
}

Copy link
Contributor

@sni sni 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

@sni
Copy link
Contributor

sni commented Sep 15, 2022

could you please rebase this PR on the latest HEAD. This should also trigger the checks again.

@sni
Copy link
Contributor

sni commented Feb 27, 2023

i just rebased this and had another look. It wont't work like this because the comment_list has only next pointer but no prev. So there is no way to remove something in the middle without walking along all the comment_list again.
I guess best would be to add a prev pointer to the comment struct as well, even though this means all NEB modules have to be rebuild again.

sni added a commit to sni/naemon-core that referenced this pull request Mar 1, 2023
This PR reworks the comment handling to improve performance when having lots of comments.

  - it add a prev pointer to the comments list so we don't have to iterate over the complete list when removing an item
  - it replaces the hash lookup to quickly access comments for specific hosts/services in favor of a new host/service attribute which holds links to the comments
  - it avoids duplicate host/service lookups for DEL_ALL_HOST_COMMENTS/DEL_ALL_SVC_COMMENTS command
  - it removes hash functions which are now unused

In general, it tries to adopt the downtime handling over to comments.

This PR is based on naemon#375.

Since the internal object structure changes, all NEB modules have to be rebuild. (We therefor increase CURRENT_NEB_API_VERSION to 6)

Signed-off-by: Sven Nierlein <[email protected]>
@sni sni mentioned this pull request Mar 1, 2023
@sni
Copy link
Contributor

sni commented Mar 1, 2023

i made some changes and created #420

@sni sni closed this Mar 1, 2023
sni added a commit that referenced this pull request Jun 22, 2023
This PR reworks the comment handling to improve performance when having lots of comments.

  - it add a prev pointer to the comments list so we don't have to iterate over the complete list when removing an item
  - it replaces the hash lookup to quickly access comments for specific hosts/services in favor of a new host/service attribute which holds links to the comments
  - it avoids duplicate host/service lookups for DEL_ALL_HOST_COMMENTS/DEL_ALL_SVC_COMMENTS command
  - it removes hash functions which are now unused

In general, it tries to adopt the downtime handling over to comments.

This PR is based on #375.

Since the internal object structure changes, all NEB modules have to be rebuild. (We therefor increase CURRENT_NEB_API_VERSION to 6)

Signed-off-by: Sven Nierlein <[email protected]>
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.

2 participants