[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8292995: improve the SA page cache [v2]
From:       Chris Plummer <cjplummer () openjdk ! org>
Date:       2022-08-30 22:57:43
Message-ID: bR8tj1sXIoShSexnU-S14a7Y1WmUbIaKQrbTCqoJCs4=.d8b0f698-dfca-4bc2-9fc6-fd46d67035f9 () github ! com
[Download RAW message or body]

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

Chris Plummer has updated the pull request incrementally with one additional commit \
since the last revision:

  Minor indent fix.

-------------

Changes:
  - all: https://git.openjdk.org/jdk/pull/10069/files
  - new: https://git.openjdk.org/jdk/pull/10069/files/9f528186..5b34d066

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=10069&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=10069&range=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/10069.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/10069/head:pull/10069

PR: https://git.openjdk.org/jdk/pull/10069


[prev in list] [next in list] [prev in thread] [next in thread]