You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/11/01 14:20:45 UTC

[GitHub] [solr] tboeghk opened a new pull request, #1156: [SOLR-16515] Remove synchronized access to cachedOrdMaps in SlowCompositeReaderWrapper

tboeghk opened a new pull request, #1156:
URL: https://github.com/apache/solr/pull/1156

   https://issues.apache.org/jira/browse/SOLR-16515
   
   The `SlowCompositeReaderWrapper` uses synchronized read and write access to its internal `cachedOrdMaps`. By using a `ConcurrentHashMap` instead of a `LinkedHashMap` as the  underlying `cachedOrdMaps` implementation and the `ConcurrentHashMap#computeIfAbsent` method to compute cache values, we were able to reduce locking contention significantly.
   
   ### Background
   
   Under heavy load we discovered that application halts inside of Solr are becoming a serious problem in high traffic environments. Using Java Flight Recordings we discovered high accumulated applications halts on the `cachedOrdMaps` in `SlowCompositeReaderWrapper`. Without this fix we were able to utilize our machines only up to 25% cpu usage. With the fix applied, a utilization up to 80% is perfectly doable.
   
   ### Description
   
   Our Solr instances utilizes the `collapse` component heavily. The instances run with 32 cores and 32gb Java heap on a rather small index (4gb). The instances scale out at 50% cpu load. We take Java Flight Recorder snapshots of 60 seconds
   as soon the cpu usage exceeds 50%.
   
   <img width="997" alt="solr-issues-scrw-locking" src="https://user-images.githubusercontent.com/557264/196221280-7eeac492-d204-497a-907e-bc261cd90530.png">
   
   During our 60s Java Flight Recorder snapshot, the ~2k Jetty threads accumulated more than 16h locking time inside the `SlowCompositeReaderWrapper` (see screenshot). With this fix applied, the locking access is reduced to cache write accesses only. We validated this using another JFR snapshot:
   
   <img width="983" alt="solr-issues-scrw-after" src="https://user-images.githubusercontent.com/557264/196221323-3ab53d23-cf23-4aee-a395-b518d4b42e54.png">
   
   ### Solution
   
   We propose the following improvement inside the `SlowCompositeReaderWrapper` removing blocking `synchronized` access to the internal `cachedOrdMaps`. The implementation keeps the semantics of the `getSortedDocValues` and `getSortedSetDocValues` methods but moves the expensive part of `OrdinalMap#build` into a producer. We use the producer to access the `ConcurrentHashMap` using the `ConcurrentHashMap#computeIfAbsent` method only.
   
   The current implementation uses the `synchronized` block not only to lock access to the `cachedOrdMaps` but also to protect the critical section between getting, building and putting the `OrdinalMap` into the cache. Inside the critical section the decision is formed, whether a cacheable value should be composed and added to the cache. 
   
   To support non-blocking read access to the cache, we move the building part of the critical section into a producer `Function`. The check whether we have a cacheable value is made upfront. To properly make that decision we had to take logic from `MultiDocValues#getSortedSetValues` and `MultiDocValues#getSortedValues` (the `SlowCompositeReaderWrapper` already contained duplicated code from those methods).
   
   ### Summary
   
   This change removes most blocking access inside the `SlowCompositeReaderWrapper` and despite it's name it's now capable of a much higher request throughput.
   
   This change has been composed together by Dennis Berger, Torsten Bøgh Köster and Marco Petris.
   
   # Checklist
   
   Please review the following and check all that apply:
   
   - [x] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
   - [x] I have created a Jira issue and added the issue ID to my pull request title.
   - [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
   - [x] I have developed this patch against the `main` branch.
   - [x] I have run `./gradlew check`.
   - [x] I have added tests for my changes.
   - [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
   


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org


[GitHub] [solr] dsmiley commented on pull request #1156: [SOLR-16515] Remove synchronized access to cachedOrdMaps in SlowCompositeReaderWrapper

Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #1156:
URL: https://github.com/apache/solr/pull/1156#issuecomment-1300871894

   I don't mean change Lucene code, I mean change SlowCompositeReaderWrapper internals only.  Yeah Multi-stuff is kind of discouraged... more of a  -- hey, use this if your algorithm can't easily loop over segments, which is probably what you should do if you cant. These Multi-classes are not going away.
   
   It's also okay to accept some duplication if you are only duplicating the Lucene code since at least then it's easier to verify we are keeping in sync with possible changes in Lucene.


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org


[GitHub] [solr] dsmiley merged pull request #1156: [SOLR-16515] Remove synchronized access to cachedOrdMaps in SlowCompositeReaderWrapper

Posted by GitBox <gi...@apache.org>.
dsmiley merged PR #1156:
URL: https://github.com/apache/solr/pull/1156


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org


[GitHub] [solr] mpetris commented on pull request #1156: [SOLR-16515] Remove synchronized access to cachedOrdMaps in SlowCompositeReaderWrapper

Posted by GitBox <gi...@apache.org>.
mpetris commented on PR #1156:
URL: https://github.com/apache/solr/pull/1156#issuecomment-1300514827

   > Hey, this looks excellent! The code looks a bit deja-vu... do you think we could & should factorize out anything common?
   
   @dsmiley We could factorize out most of `SlowCompositeReaderWrapper::getSortedDocValues` and `SlowCompositeReaderWrapper::getSortedSetDocValues` to `MultiDocValues` just like it was. But to me it looks like as the two `MultiDocValues` methods were used in `SlowCompositeReaderWrapper` only. So I'm not sure that it would be worth the effort. Moreover since the usage of `MultiDocValues` seems to be discouraged anyway.


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org


[GitHub] [solr] DennisBerger1984 commented on pull request #1156: [SOLR-16515] Remove synchronized access to cachedOrdMaps in SlowCompositeReaderWrapper

Posted by GitBox <gi...@apache.org>.
DennisBerger1984 commented on PR #1156:
URL: https://github.com/apache/solr/pull/1156#issuecomment-1299716722

   ![Screenshot_20221102_084057](https://user-images.githubusercontent.com/69144692/199429777-e3f51cb5-406c-4a15-9441-db3537e389ac.png)
   
   It's a WrappedQuery object.


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org


[GitHub] [solr] mpetris commented on pull request #1156: [SOLR-16515] Remove synchronized access to cachedOrdMaps in SlowCompositeReaderWrapper

Posted by GitBox <gi...@apache.org>.
mpetris commented on PR #1156:
URL: https://github.com/apache/solr/pull/1156#issuecomment-1301979984

   OK, got it. We refactored the implementation a bit to seperate the algorithm from the access to SortedDocValues and SortedSetDocValues. This allows us to have the algorithm only once. It is a different style now compared to the style from MultiDocValues. I like the refactored one better but it depends a bit on ones taste.


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org