Aggregate files from the same segment into a single Arena #13570

ChrisHegarty · 2024-07-14T19:47:14Z

This commit adds a ref counted shared arena to support aggregating segment files into a single Arena.

uschindler

I like the code much more:

the attachment is a good idea for the meantime to clearly separate compilation units
the refocunting looks much better here - thanks. That was my main problem with the previous approach by @magibney

lucene/core/src/java/org/apache/lucene/store/MMapDirectory.java

lucene/core/src/java21/org/apache/lucene/store/RefCountedSharedArena.java

magibney

This looks good (sorry about the lack of write access to the original PR -- institutional repo, not an option).

attachment() is quite nice.

Unless I'm mistaken, this PR has ended up with refcounting that's nearly identical to the current state of #13555 (though it fixes a bug in the other PR's acquire, where CAS must be used in place of getAndIncrement())? It's a small point, but I think CAS is not necessary for release() here.

My only other concern applies here equally as to my earlier PR:

      // TODO: what if the associated Directory is used for some purpose other than Lucene
      //  index files, and some of the filenames happen to be patterned so that a "segment"
      //  can be parsed from them? That could result in files being spuriously grouped
      //  together in the same arena. This would only practically be a problem if such a
      //  "segment" was never closed (refCt never 0), _and_ enough files associated with that
      //  "segment" were opened and closed (without ever closing the associated Arena)
      //  to exhaust virtual memory space.

lucene/core/src/java21/org/apache/lucene/store/RefCountedSharedArena.java

uschindler

OK from my side, only the missing final (maybe it should be private, too) should be fixed.

The Generics are in fact a bit strange, but due to the MethodHandle and casting after ctor it works woth Object as type. I would have wished to get rid of the generic type. Maybe the field in MMapDirectory should use "?" as type, not "Object"? Just an idea.

Let me think ab bit about it, but it is a workaround for something that could be fixed later when the whole provider gets obsolete (Java 24 or 25 LTS).

uschindler · 2024-07-15T15:06:58Z

Hi,
we also get Updates on the JDK fixes: openjdk/jdk#20158 (see issue about more details and some improvements Solr and/or Elasticsearch/Opensearch should do).

magibney · 2024-07-15T15:17:37Z

To expand a bit on the concern I raised above:

IIUC, in order for this to work properly (guaranteed to not potentially leak virtual memory address space) it depends on segment filename usage patterns and segment lifecycle. I think we may either need to:

provide a way to configure specific MMapDirectory instances to bypass this Arena pooling, or
disallow use of MMapDirectory for anything other than Lucene index files (because of the potential for collision of filename patterns that could be parsed as "segments", potentially allowing an Arena to live forever, accumulating an unlimited number of associated files.

Am I missing something -- is this not a concern?

uschindler · 2024-07-15T15:30:13Z

To expand a bit on the concern I raised above:

IIUC, in order for this to work properly (guaranteed to not potentially leak virtual memory address space) it depends on segment filename usage patterns and segment lifecycle. I think we may either need to:

provide a way to configure specific MMapDirectory instances to bypass this Arena pooling, or

disallow use of MMapDirectory for anything other than Lucene index files (because of the potential for collision of filename patterns that could be parsed as "segments", potentially allowing an Arena to live forever, accumulating an unlimited number of associated files.

Am I missing something -- is this not a concern?

In addition we could add a separate ctor option to disable grouping.

In addition I would go for option #2: Like for the segment main file it should also not use grouped segments for any other file that is not a correct segment number (it could validate the filename using the base32 hash starting with "_" or whatever this is).

Maybe add a protected method to MMapDirectory that checks if a filename is a segement file) and pass this one as Predicate<String> to the provider interface.

uschindler · 2024-07-15T15:42:28Z

We may need some other logic: We have some files that can change without the segment name changing? How do we handle *.del, softdelete or docValues in-place update files? If they correspond to the same segment, they would only be closed when actually the whole segment is closed after full refresh.

This should be easily testable on windows: Files that have mmapped areas can't be deleted, so we may cause issues when closing deletion files.

But on the other hand: Aren't ".del" files not READONCE!?

uschindler

I changed to request changes, because we should confirm the deletion/softdeletes and docvalues updates.

Maybe the file pattern should really be a Predicate to separate the implementation. Maybe add a method to return protected Optional<String> getArenaGroupingKey(String filename) (or as Function<String,Optional<String>>), which returns empty optional if no grouping allowed or the segment number otherwise as key.

uschindler · 2024-07-15T15:47:26Z

I changed to request changes, because we should confirm the deletion/softdeletes and docvalues updates.

Maybe the file pattern should really be a Predicate to separate the implementation. Maybe add a method to return protected Optional<String> getArenaGroupingKey(String filename) (or as Function<String,Optional<String>>), which returns empty optional if no grouping allowed or the segment number otherwise as key.

This could be similarily implemented like the setPreload(BiPredicate<String,IOContext>) method.

magibney · 2024-07-15T15:50:12Z

heh, was just writing to suggest something similar (Function<String,Optional<String>>). SGTM.

uschindler · 2024-07-15T15:56:02Z

It could even be simpler by passing the grouping key to the provider method directly in addition to filename. Then the code that figures out which group is used can reside in MMapDirectory only.

dsmiley

Coincidentally I'm working on a similar ref-counting in a ConcurrentHashMap thing in Solr for something else (not for Arena) so it's nice to review this; bit of deja-vu.

lucene/core/src/java/org/apache/lucene/store/MMapDirectory.java

lucene/core/src/java21/org/apache/lucene/store/RefCountedSharedArena.java

lucene/core/src/java21/org/apache/lucene/store/MemorySegmentIndexInputProvider.java

dsmiley

Can someone take a stab at summarizing this for CHANGES.txt, thus avoiding details a reader is unlikely to know about? Like me :-). How will a user of Lucene benefit?

magibney

This looks good; I left a few comments, but the only one that really has to happen I think is the removal of the ISE in acquire().

lucene/core/src/java21/org/apache/lucene/store/RefCountedSharedArena.java

lucene/core/src/java/org/apache/lucene/store/MMapDirectory.java

lucene/core/src/java21/org/apache/lucene/store/RefCountedSharedArena.java

lucene/core/src/java/org/apache/lucene/store/MMapDirectory.java

lucene/core/src/java21/org/apache/lucene/store/MemorySegmentIndexInputProvider.java

uschindler

Looks fine, some suggestions.

To make the javadocs visible the static sysprop name constant should be public.

lucene/core/src/java/org/apache/lucene/store/MMapDirectory.java

uschindler

Looks fine now.

ChrisHegarty · 2024-07-24T13:18:05Z

This looks like it's in good shape now. @magibney Any final comments? Otherwise, I plan to merge tomorrow. And then figure out how to backport!

magibney

Looks good, thanks! (aside from a javadoc typo that I happened to notice)

lucene/core/src/java21/org/apache/lucene/store/RefCountedSharedArena.java

uschindler · 2024-07-24T16:32:15Z

Otherwise, I plan to merge tomorrow. And then figure out how to backport!

Code duplication with Arena vs. MemorySession hell!

lucene/core/src/java/org/apache/lucene/store/MMapDirectory.java

uschindler · 2024-07-25T10:45:48Z

It's finally done. Thanks to all and help while implementing my idea!

uschindler · 2024-08-01T22:51:11Z

Do we have a backport PR? Should I work on it?

uschindler · 2024-08-22T16:11:31Z

@ChrisHegarty the PR ist listed in the 9.x section of changes. I could work on a backport (possibly tomorrow). I just want to make sure you haven't started.

I am not yet sure how to handle java 20 and java 19. I may possibly leave those as they are.

ChrisHegarty · 2024-08-22T18:40:03Z

@ChrisHegarty the PR ist listed in the 9.x section of changes. I could work on a backport (possibly tomorrow). I just want to make sure you haven't started.

I am not yet sure how to handle java 20 and java 19. I may possibly leave those as they are.

Yeah, I had the same thought too - to just do this for JDK 21+.

I’ve not started, but it is on my todo list. I’ll raise a PR for it tomorrow, and if you have time maybe you could help review.

uschindler · 2024-08-23T08:42:24Z

We may need to change the changes entry to tell that to enduser.

Backport of #13570 This commit adds a ref counted shared arena to support aggregating segment files into a single Arena. The backport only supports the JDK 21 version of the code - JDK 19 and 20 specific versions do not aggregate files so therefore remain the same. The MemorySession/SegmentScope/Arena APIs are different in 19 and 20.

RefCountedSharedArena

2d6af43

ChrisHegarty requested a review from uschindler July 14, 2024 19:47

ChrisHegarty changed the title ~~Add RefCountedSharedArena~~ Aggregate files from the same segment into a single Arena Jul 14, 2024

ChrisHegarty mentioned this pull request Jul 14, 2024

Group memory arenas by segment to reduce costly Arena.close() #13555

Closed

uschindler mentioned this pull request Jul 15, 2024

8335480: Only deoptimize threads if needed when closing shared arena openjdk/jdk#20158

Closed

3 tasks

uschindler requested changes Jul 15, 2024

View reviewed changes

ChrisHegarty added 4 commits July 15, 2024 11:25

remove optional

fa0a962

use AtomicInteger

fe65dfe

improve test

f68a917

Merge branch 'main' into refCntArena

7d2124f

magibney reviewed Jul 15, 2024

View reviewed changes

lucene/core/src/java21/org/apache/lucene/store/RefCountedSharedArena.java Outdated Show resolved Hide resolved

lucene/core/src/java21/org/apache/lucene/store/RefCountedSharedArena.java Outdated Show resolved Hide resolved

uschindler approved these changes Jul 15, 2024

View reviewed changes

private final

d68e26e

uschindler mentioned this pull request Jul 15, 2024

Examine performance of individual data accessor methods of MemorySegmentIndexInput when IndexInputs are closed in other threads (deoptimizations,...) #13325

Open

uschindler approved these changes Jul 15, 2024

View reviewed changes

simplify with decrementAndGet

6b6d7c1

uschindler requested changes Jul 15, 2024

View reviewed changes

dsmiley reviewed Jul 15, 2024

View reviewed changes

lucene/core/src/java21/org/apache/lucene/store/MemorySegmentIndexInputProvider.java Show resolved Hide resolved

ChrisHegarty added 2 commits July 16, 2024 11:37

rename to onClose

f39c401

add override

e65c4cb

ChrisHegarty mentioned this pull request Jul 16, 2024

Ensure to use IOContext.READONCE when reading segment files #13574

Merged

dsmiley reviewed Jul 16, 2024

View reviewed changes

magibney requested changes Jul 22, 2024

View reviewed changes

dsmiley reviewed Jul 22, 2024

View reviewed changes

lucene/core/src/java21/org/apache/lucene/store/RefCountedSharedArena.java Outdated Show resolved Hide resolved

ChrisHegarty added 4 commits July 23, 2024 10:20

ref counted review comments

875bcb1

typo

77819db

Merge branch 'main' into refCntArena

9b859de

add sys prop

5576c81

ChrisHegarty requested a review from magibney July 23, 2024 11:47

uschindler approved these changes Jul 23, 2024

View reviewed changes

lucene/core/src/java/org/apache/lucene/store/MMapDirectory.java Show resolved Hide resolved

lucene/core/src/java21/org/apache/lucene/store/MemorySegmentIndexInputProvider.java Outdated Show resolved Hide resolved

ChrisHegarty added 2 commits July 24, 2024 11:18

move sys prop and gen grouping

80f3d5c

revert accidental

8a6eba0

ChrisHegarty requested a review from uschindler July 24, 2024 10:25

uschindler reviewed Jul 24, 2024

View reviewed changes

remove optional, public sys prop, and review comments

99262e2

uschindler approved these changes Jul 24, 2024

View reviewed changes

magibney approved these changes Jul 24, 2024

View reviewed changes

lucene/core/src/java21/org/apache/lucene/store/RefCountedSharedArena.java Outdated Show resolved Hide resolved

fix typo

aeab592

dsmiley reviewed Jul 24, 2024

View reviewed changes

lucene/core/src/java/org/apache/lucene/store/MMapDirectory.java Show resolved Hide resolved

Merge branch 'main' into refCntArena

e86191c

ChrisHegarty merged commit b4fb425 into apache:main Jul 25, 2024
3 checks passed

ChrisHegarty deleted the refCntArena branch July 25, 2024 09:12

uschindler added this to the 9.12.0 milestone Aug 22, 2024

ChrisHegarty mentioned this pull request Aug 23, 2024

[9.x] Aggregate files from the same segment into a single Arena #13682

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aggregate files from the same segment into a single Arena #13570

Aggregate files from the same segment into a single Arena #13570

ChrisHegarty commented Jul 14, 2024 •

edited

Loading

uschindler left a comment

magibney left a comment

uschindler left a comment

uschindler commented Jul 15, 2024 •

edited

Loading

magibney commented Jul 15, 2024

uschindler commented Jul 15, 2024

uschindler commented Jul 15, 2024 •

edited

Loading

uschindler left a comment

uschindler commented Jul 15, 2024

magibney commented Jul 15, 2024

uschindler commented Jul 15, 2024 •

edited

Loading

dsmiley left a comment

dsmiley left a comment

magibney left a comment

uschindler left a comment

uschindler left a comment

ChrisHegarty commented Jul 24, 2024 •

edited

Loading

magibney left a comment

uschindler commented Jul 24, 2024 •

edited

Loading

uschindler commented Jul 25, 2024

uschindler commented Aug 1, 2024

uschindler commented Aug 22, 2024

ChrisHegarty commented Aug 22, 2024

uschindler commented Aug 23, 2024

Aggregate files from the same segment into a single Arena #13570

Aggregate files from the same segment into a single Arena #13570

Conversation

ChrisHegarty commented Jul 14, 2024 • edited Loading

uschindler left a comment

Choose a reason for hiding this comment

magibney left a comment

Choose a reason for hiding this comment

uschindler left a comment

Choose a reason for hiding this comment

uschindler commented Jul 15, 2024 • edited Loading

magibney commented Jul 15, 2024

uschindler commented Jul 15, 2024

uschindler commented Jul 15, 2024 • edited Loading

uschindler left a comment

Choose a reason for hiding this comment

uschindler commented Jul 15, 2024

magibney commented Jul 15, 2024

uschindler commented Jul 15, 2024 • edited Loading

dsmiley left a comment

Choose a reason for hiding this comment

dsmiley left a comment

Choose a reason for hiding this comment

magibney left a comment

Choose a reason for hiding this comment

uschindler left a comment

Choose a reason for hiding this comment

uschindler left a comment

Choose a reason for hiding this comment

ChrisHegarty commented Jul 24, 2024 • edited Loading

magibney left a comment

Choose a reason for hiding this comment

uschindler commented Jul 24, 2024 • edited Loading

uschindler commented Jul 25, 2024

uschindler commented Aug 1, 2024

uschindler commented Aug 22, 2024

ChrisHegarty commented Aug 22, 2024

uschindler commented Aug 23, 2024

ChrisHegarty commented Jul 14, 2024 •

edited

Loading

uschindler commented Jul 15, 2024 •

edited

Loading

uschindler commented Jul 15, 2024 •

edited

Loading

uschindler commented Jul 15, 2024 •

edited

Loading

ChrisHegarty commented Jul 24, 2024 •

edited

Loading

uschindler commented Jul 24, 2024 •

edited

Loading