On Thu, 7 Nov 2024 12:48:12 GMT, Afshin Zafari <azaf...@openjdk.org> wrote:
>> - `VMATree` is used instead of `SortedLinkList` in new class >> `VirtualMemoryTrackerWithTree`. >> - A wrapper/helper `RegionTree` is made around VMATree to make some calls >> easier. >> - Both old and new versions exist in the code and can be selected via >> `MemTracker::set_version()` >> - `find_reserved_region()` is used in 4 cases, it will be removed in >> further PRs. >> - All tier1 tests pass except one ~that expects a 50% increase in committed >> memory but it does not happen~ https://bugs.openjdk.org/browse/JDK-8335167. >> - Adding a runtime flag for selecting the old or new version can be added >> later. >> - Some performance tests are added for new version, VMATree and Treap, to >> show the idea and should be improved later. Based on the results of >> comparing speed of VMATree and VMT, VMATree shows ~40x faster response time. > > Dear @tstuefe, this PR has been reviewed couple of rounds. Would you please, > give your feedback? > Thanks. Hi @afshin-zafari, I'll take a look next week. Could you please remove draft state, so that the discussions are public on the ML? Could you also please, to make reviewing easier and for documentation, describe the desired behavior? You should also add this description (or reuse, could be the same text) to the header/start of the implementation. Detailed questions are: 1. where does the data structure live? E.g. "Its a global data structure". Important here is the distinction to the ZGC file mapping tracker. 2. How is the data structure guarded? (locking question) 3. how are memory reservations realized (what tree operations?) E.g. "creates a VMATree memory section with state=reserved and the given tag/stack combination". 3.1 Do we allow reservations to overlap with existing sections? If no, why? If yes, under which conditions, and what happens to the old sections? 4. how is memory commit realized? 4.2. What happens if I specify tag and stack? Any restrictions then? 4.1. What happens if I omit tag and stack on commit? What are the restrictions? 5. same for uncommit 6. How is release done? 7. How do we find memory sections? 8. Do we allow modifying the tag? (See @jdksjolen set_tag PR) Here is what I think how this should work: NMT should normally not put any restrictions on the use of mmap APIs. It should just faithfully track what we do. However, we may want a "paranoid" mode where we bail out/warn if something looks fishy. 3. reservations should create a new tree section with the given flag/stack 3.1 there should be no restrictions. If there are old sections in that range, they are replaced. However, with paranoid mode enabled, I would warn if we reserve over committed sections because that would replace the mapping in reality. Remapping just reserved but uncommitted sections should be fine, I think. I think we do this sometimes. 4. commit should work differently when giving tag+stack, or when tag and stack are omitted. 4.1 if tag+stack are given, commit just creates a single new section across the range with the given tag + stack. That should always work, regardless of whats there before (paranoid off). With paranoid=on, it should warn if we commit over already Committed sections, since that destroys those sections. 4.2) committing when tag+stack are omitted. As in "please commit what I reserved before, and just with the same tag and stack". In that case, commit just changes the tag for the range. That may or may not cause sections to coalesce (if changing the tag causes neighboring sections to have the same set of properties) or be created (if you commit middle of an existing section, and the set of properties now differs). Restrictions: there should be a clear unconditional restriction that this only works if there are no Released sections in that range, since a Released section (address holes) has no tag+stack to copy from. 5. Uncommit should never require tag+stack. It should change the state of the range to "Reserved" and leave tag+stack alone. Again, cannot work across address space holes. It can cause the splintering of existing sections, if we uncommit the middle of a committed section. 6. Releasing should just create a new section with State=Released, tag=mtNone and no stack. WDYT. I think it is a must to have all of this clearly specified before starting to implement, because the code is complex and one needs to know this logic beforehand. ------------- PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2463896881
