That’s interesting and using
IntIndexCollectorUncompressed is a simple change.
Is LongIndexCollector object2position also contended? LongIndexCollectorUncompressed would also be an option.
However, these choices will use more memory. With 16,000,000 objects, the object2classId index could increase from 48MB to 64MB and object2position from perhaps
60MB to 128MB.
We could select depending on how much heap is available. We do something similar: to disable parallel parsing when memory is short. However, I have found that MAT
can get stuck when the heap size is on the boundary of being too small.
org.eclipse.mat.tests.regression.Application -performance
can run with heap between two -Xmx values using a binary search to find the smallest heap which works.
I’d like to fix ‘Bug 581932 - ArrayIndexOutOfBoundsException in ArrayIntCompressed on beforePass2 parsing’ before making further changes to the HPROF
parser. Do you have an idea for solving this bug?
Andrew Johnson
From: Yi Yang
Sent: Thursday, June 1, 2023 4:28 AM
To: Andrew Johnson; Memory Analyzer Dev list <mat-dev@xxxxxxxxxxx>
Cc: jkoch
Subject: Re: [mat-dev] Is it necessary to synchronize IntIndexCollector.get/set during Pass2Parser?
Hi Jason and Andrew,
> To confirm, are you referring to this line?
Yes, this is exactly what I refer to.
According to profiling data for wall time and CPU time, there is high contention in IntIndexCollection.set and get, which takes up a significant amount of time.
We can break down the problem into two parts:
- `synchronized(){}` between IntIndexCollection.set and IntIndexCollection.get: there is no time overlap between set and get, so we don't need to synchronize get.
- synchronization within IntIndexCollection.set: set is called concurrently, and as aforementioned, even if two indices are different, the final byte[] range may
overlap, making it difficult to remove the lock.
Given that IntIndexCollection.set is highly contended, it is without a doubt that the lock will significantly impact performance. We need to find another way to
handle this issue.
Then I replace ArrayIntCompressed with an uncompressed int[] version and remove all locks within IntIndexCollection.set and get methods. When testing the parsing
of 40GB heap, I achieved a 32.5% performance improvement stably. But it seems that IntIndexCollectorUncompressed mentioned by Andrew is what I want
Time elapsed: 1,898.853 s
Time elapsed: 1,868.729 s
Time elapsed: 1,845.501 s
--remove_locks_and_use_int[]_in_ArrayIntCompressed
Time elapsed: 1,280.707 s
Time elapsed: 1,282.467 s
--use IntIndexCollectorUncompressed
Time elapsed: 1,318.243 s
Yi Yang
------------------------------------------------------------------
Send Time:2023 Jun. 1 (Thu.) 04:47
Subject:RE: [mat-dev] Is it necessary to synchronize IntIndexCollector.get/set during Pass2Parser?
Thank you for your interest in improving the performance of MAT.
The parsing code was enhanced to have some parallelism, and some of the index code was enhanced to be thread safe.
The index code in org.eclipse.mat.parser is an MAT API, so we can’t really make incompatible changes as an adopter of MAT might already be using it and presuming some thread safety. We
should improve the Javadoc to explain what is safe.
I think your example is for code in HprofParserHandlerImpl.java such as:
// log address
object2classId.set(index, classIndex);
object2position.set(index, object.filePosition);
Yes, the index is separate for separate threads, but IntIndexCollector then chooses a page based on the index giving an ArrayIntCompressed object in which to set the value. This internally
has a byte array but the value can be spread across multiple bytes as the class index is known to have leading zeroes so only certain bits need to be stored. This means two threads could access the same byte in the byte array. So, synchronization is needed.
It might be possible to use AtomicIntegerArray instead of a byte[] but that could be slower. Is there contention on the lock? The page size is 1000000 so if the number of objects in the heap is smaller than this then they will all end up on the same ArrayIntCompressed.
If the number is much larger then there might still be a lot of contention as the objects in the HPROF are often in address order so the threads might be processing objects with a similar object index, so even reducing the page size might not help. Is using
IntIndexCollectorUncompressed any faster (at the cost of more memory)? There may be similar problems with the object2position index.
For more performance work we would like some reproducible tests – there are some performance tests in org.eclipse.mat.tests but we don’t run them.
Regards,
Andrew Johnson
Hi all, when building hprof index, MAT first creates Pass2Parser and concurrently parses objects and adds them to ArrayIntCollector. Then, in the subsequent fillIn
routine, it reads objects from ArrayIntCollector in order and writes them to index file. The entire process is shown below:
- Pass2Parser.readSegment
- addObject to IntIndexCollector in parallel by calling IntIndexCollector.set(key, value)
- write IntIndexCollector to file by calling IntIndexCollector.get(key)
It seems that there is no time overlap between IntIndexCollector.get and IntIndexCollector.set? Additionally, when calling ArrayIntCollector.set(key, value) in addObject,
the key is the object's ID and is unique. So, is it really necessary to synchronize get/set under synchronized protection? I observed that addObject has considerable performance overhead, and removing this lock protection results in a 19.3% performance improvement.
Unless otherwise stated above:
IBM United Kingdom Limited
Registered in England and Wales with number 741598
Registered office: PO Box 41, North Harbour, Portsmouth, Hants. PO6 3AU
Unless otherwise stated above:
IBM United Kingdom Limited
Registered in England and Wales with number 741598
Registered office: PO Box 41, North Harbour, Portsmouth, Hants. PO6 3AU
|
