[GitHub] [trafficserver] shinrich opened a new pull request #7848: Remove bucket search from IntrusiveHashMap::erase

[GitBox <gi...@apache.org>]

shinrich opened a new pull request #7848:
URL: https://github.com/apache/trafficserver/pull/7848


   Noticed this while working with the origin session pool tracking down a problem with too many origin connections in the pool.  In perftop both FQDNLinkage::find and IPLinkage::find were showing up in the top 20. 
   
   Looking at erase taking a value argument (instead of an iterator), the erase logic was doing a find() first to make sure the value was still there.  Even though the value has the next/prev links intrusively embedded in the object. 
   
   This code change makes the value-based erase act like the iterator based erase.  It seems that we should have a different call if we are not sure whether the object is in the container or not.  In the case of the origin session pools, we do know that it is in the pool when we get ready to erase it.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficserver] shinrich merged pull request #7848: Remove bucket search from IntrusiveHashMap::erase

[GitBox <gi...@apache.org>]

shinrich merged pull request #7848:
URL: https://github.com/apache/trafficserver/pull/7848


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficserver] SolidWallOfCode commented on pull request #7848: Remove bucket search from IntrusiveHashMap::erase

[GitBox <gi...@apache.org>]

SolidWallOfCode commented on pull request #7848:
URL: https://github.com/apache/trafficserver/pull/7848#issuecomment-845203806


   Because you don't have the iterator, you have the value. This comes up when the session is found in table Alpha, and it needs to be removed from Alpha and Bravo. Because it's intrusive, there's no need to do a find on Bravo. The previous code was being too cautious in verifying the value is really in the table. @shinrich convinced me it would be better to provide a separate method (e.g. `contains`) if the call site wants the extra security. Otherwise assume the value is really in the table.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficserver] SolidWallOfCode commented on pull request #7848: Remove bucket search from IntrusiveHashMap::erase

[GitBox <gi...@apache.org>]

SolidWallOfCode commented on pull request #7848:
URL: https://github.com/apache/trafficserver/pull/7848#issuecomment-842701174


   Why not
   ```
     auto loc = this->iterator_for(v);
     if (loc != this->end()) {
       this->erase(loc);
       return true;
     }
     return false;
     ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficserver] zwoop commented on pull request #7848: Remove bucket search from IntrusiveHashMap::erase

[GitBox <gi...@apache.org>]

zwoop commented on pull request #7848:
URL: https://github.com/apache/trafficserver/pull/7848#issuecomment-844165783


   I'm confused ... There is a 
   
   ```
   template <typename H>
   auto
   IntrusiveHashMap<H>::erase(iterator const &loc) -> iterator
   ```
   
   Which does what you want here I think ? Why not change the call where you ran into this (where you know it's in the container), and call with the iterator argument instead? And leave the ::erase(value) as-is ?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org