On Mon, 29 Aug 2022 20:13:46 GMT, Chris Plummer <[email protected]> wrote:
> The page caching support in SA is woefully dated. I think it has stayed the
> same for over 20 years when it was originally done for solarix-x86. This code
> has been replicated for every port. Currently all ports only have a 16mb
> cache. They use 4k pages and there are 4k of them.
>
> I think the 4k page size is fine. The following comment appears in all the
> ports:
>
> // ... This is a cache of 4096 4K pages, or 16 MB. The page
> // size must be adjusted to be the hardware's page size.
> // (FIXME: should pick this up from the debugger.)
>
> I disagree with this. Matching the possibly very large hardware page size (I
> think maybe they meant OS page size) would require the SA page cache to be
> very very large, using a lot of java heap space. It would also require a lot
> of unnecessary copying from the debuggee process's memory. There's no reason
> for the SA cache's page size to match the OS page size.
>
> However, 16mb seems very small. I tried 256mb and this gave about a 10%
> performance improvement in a heap dump, and is still fairly small, so I think
> it is a reasonable adjustment.
>
> Another comment you see in all the ports (copied from solaris-x86) is:
>
> // FIXME: re-test necessity of cache on Linux, where data
> // fetching is faster
> // Cache portion of the remote process's address space.
> // Fetching data over the socket connection to dbx is slow.
> // Might be faster if we were using a binary protocol to talk to
> // dbx, but would have to test. For now, this cache works best
> // if it covers the entire heap of the remote process. FIXME: at
>
> At least on linux the cache is definitely needed. I turned it off and a heap
> dump took 9x longer. Also I think covering the entire heap is overkill, and I
> doubt was ever being done given how small the cache is. So I think this
> comment can just be removed.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/windbg/WindbgDebuggerLocal.java
line 123:
> 121:
> 122: if (useCache) {
> 123: initCache(4096, parseCacheNumPagesProperty(1024 * 64));
Not a review. Just a minor comment.
The indent does not match the above lines.
As I see, the indent in the updated files is not unified. There is even indent
3.
-------------
PR: https://git.openjdk.org/jdk/pull/10069
