Bug 582823 - query: IndexWriter.LongIndex reverse should also use a threadsafe cache?
Summary: query: IndexWriter.LongIndex reverse should also use a threadsafe cache?
Status: CLOSED MOVED
Alias: None
Product: MAT
Classification: Tools
Component: Core (show other bugs)
Version: 1.15   Edit
Hardware: PC All
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Project Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-12-31 17:04 EST by Jason Koch CLA
Modified: 2024-05-08 17:06 EDT (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jason Koch CLA 2023-12-31 17:04:10 EST
I notice that IndexReader.LongIndexReader::reverse uses a ConcurrentHashMap to track the binary search cache. I see similar code in IndexWriter.LongIndex but it hasn't been modified to use a CHM.

I think it should be swapped, and have a very similar patch running on our local branch.

Was there a reason for this? Happy to submit patch for it to use CHM.
Comment 1 Andrew Johnson CLA 2024-01-02 04:22:19 EST
Do you know where IndexWriter.LongIndex.reverse() is used?
It wasn't changed because it didn't seem to be used in a multi-threaded way.

We could go through the API working out which ones are thread-safe, but might need input from the adopter community to see what is important.
Comment 2 Jason Koch CLA 2024-01-02 14:38:44 EST
> Do you know where IndexWriter.LongIndex.reverse() is used?

Good question. I modified the code to throw an exception, and only and 8 tests failed:
- longIndexCollector4[index size 6]
- longIndexCollector4[index size 999,999]
- longIndexCollector4[index size 1,000,000]
- longIndexCollector4[index size 1,000,001]
- longIndexCollector4[index size 1,999,999]
- longIndexCollector4[index size 2,000,000]
- longIndexCollector4[index size 2,000,001]
- longIndexCollector4[index size 1,000]

So it would appear the code is currently unused in any of the test cases.

Further, when I look at the code paths.
- LongIndex is abstract static class.
- IndexWriter.LongIndexCollector extends it and uses the LongIndex impl
- IndexWriter.LongIndexStreamer (and PosIndexStreamer) extends it and uses the LongIndex impl
- IndexReader.LongIndexReader extends it and provides its own reverse() implementation

As far as I can tell, the writeTo() all return a LongIndexReader which has its own impl. The LongIndexCollector and LongIndexStreamer are never used with a reverse() call. Identifier::reverse is also called but it has its own implementation too!

> We could go through the API working out which ones are thread-safe, but might need input from the adopter community to see what is important.

On one hand, I think upgrading something not-thread-safe to thread-safe seems wise. On the other, Hyrum's Law is also at play here, given majority of the parser classes are not thread-safe and only being made thread-safe as a result of demands from hprof parser.

I'm only aware of one extension (Mat-Calcite-Plugin), and one app over the top (JIFA) that rely on MAT. Is there a catalogue available?
Comment 3 Jason Koch CLA 2024-01-02 14:39:36 EST
So it would appear the code is currently unused in any of the *end to end* / integration test cases.
Comment 4 Eclipse Webmaster CLA 2024-05-08 17:06:43 EDT
This issue has been migrated to https://github.com/eclipse-mat/org.eclipse.mat/issues/47.