Add DocValuesProducers for releasing memory when close index

[luyuncheng]

Description

in #1885 we talk about a method that we need release memory when a producer closed. so i open this PR and added KNN80DocValuesProducer. this producer can release memory when reader closed a segment.

also i added refCount as the comments cares about.

i think this pr is the 1st step, we only added a producers, because we talked in #1885, that we need get binaryDocValues in DocValuesProducers from native engine.

i see there is an #1853 on going, so i prefer to in next step to read binaryDocValues from native engines.

Related Issues

#1885

Check List

• New functionality includes testing.
• New functionality has been documented.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[navneet1v]

@luyuncheng can we fix the build CIs. BWC CIs is failing across other PRs will check whats happening there.

[jmazanec15]

Thanks for making this @luyuncheng. Overall looks pretty good. A few comments. I think it is okay to initially focus on DocValues and then add functionality for KnnVectorsFormat in another PR (and get rid of file watcher in this PR).

[jmazanec15]

For future extendability, could we check if this is a k-NN file and open it from the directory? I know we typically dont open k-NN files via openInput, but if we do, this might fail:

        if (KNNEngine.getEnginesThatCreateCustomSegmentFiles().stream().anyMatch(engine -> name.endsWith(engine.getCompoundExtension()))) {
            return directory.openInput(name, context);
        }

        return delegate.openInput(name, context);

[navneet1v]

@jmazanec15 why we want to do this? this can lead to unnecessary mapping of the file in memory. My thoughts will be not to do this.

[jmazanec15]

I dont think anyone should call openInput on k-NN files. But if they do, return delegate.openInput(name, context); will throw exception because file isnt actually in compound file

[navneet1v]

I am not sure if exception will come. @luyuncheng have you seen an exception here? I think it will open the input.

But even if there is no exception we should not open the input, it is just unnecessary mapping of file. One we start to use IndexInput to read the graph file(ref: #1951) we can start opening the files here.

[luyuncheng]

i did not find any exception. but i do want to implement an nativeEngine indexInput like #1951 says.
how about:

1. we return return delegate.openInput(name, context);
2. and added a assert when name is native engines.
3. also added an TODO comments here: TODO: using the native engine IndexInput.

@navneet1v @jmazanec15

[jmazanec15]

@navneet1v See how we write: https://github.com/luyuncheng/k-NN-1/blob/DocValuesProducers/src/main/java/org/opensearch/knn/index/codec/KNN80Codec/KNN80CompoundFormat.java#L48-L53. We dont use the delegate to write the file, we use directory directly.

[navneet1v]

@jmazanec15 I understand we would be directly writing the file. I have tested a small code change where in KNNWeight class I was opening .hnsw file using IndexInput and it worked. I didn't get the exception. So want to know when you say there will be exception what will be that exception.

Neverthless I believe we should not even open the nativeIndex files in the producer as it will unnecessary map the file on RAM.

[jmazanec15]

In your test was it an .hnsw file or .hnswc? I think .hnswc should fail with this line

[kotwanikunal]

Added some thoughts related to the cache free up: #1885 (comment)

[jmazanec15]

One minor comment - other than that looks good!

[luyuncheng]

Can we check if FilterDirectory unwraps to FSDirectory before casting? If it does not, can we warn log and NO-OP? My main concern here is around remove directory implementations.

@jmazanec15 at 2bcc139 how about added try catch ClassCastException

[jmazanec15]

That looks good. Thanks approving!

[navneet1v]

@luyuncheng can you please look at the comment added reply on them.

[luyuncheng]

@luyuncheng can you please look at the comment added reply on them.

@navneet1v Sorry for the late reply, i just get back from vacation. i'll do it ASAP.

[jmazanec15]

LGTM

[luyuncheng]

@jmazanec15 @navneet1v Thanks guys for reviewing

[0ctopus13prime]

Hi @luyuncheng
Could you share why we don't include a field having 'model_id' in its field attribute into indexPathMap??
Wondering having 'model_id' in its field attribute mean something else??

Link : https://github.com/opensearch-project/k-NN/blob/main/src/main/java/org/opensearch/knn/index/codec/KNN80Codec/KNN80DocValuesProducer.java#L140

// Only Native Engine put into indexPathMap
KNNEngine knnEngine = getNativeKNNEngine(field);
if (knnEngine == null) {
    continue;
}

private KNNEngine getNativeKNNEngine(@NonNull FieldInfo field) {
    final String modelId = field.attributes().get(MODEL_ID);
    if (modelId != null) {
        return null; <------------- This
    }
    ...

[luyuncheng]

Could you share why we don't include a field having 'model_id' in its field attribute into indexPathMap??
Wondering having 'model_id' in its field attribute mean something else??

because in model engine, do not need docValuesProducer for merge. so i filter it out

[0ctopus13prime]

Could you share why we don't include a field having 'model_id' in its field attribute into indexPathMap??
Wondering having 'model_id' in its field attribute mean something else??

because in model engine, do not need docValuesProducer for merge. so i filter it out

@luyuncheng
Thank you for the response.
Could you provide more info about it? Still not clear and hard for me to build a connection between two.
Specificlly,

1. What is model engine??
2. What do we do with a model engine??
3. How we differentiate model engine from the plain vector engine (ex: Hnsw)??
4. Why a model engine does not need a resource clean up in cache manager when DVReader is being closed??

Thank you!

[luyuncheng]

@0ctopus13prime

What is model engine??

i think only faiss can have a model engine?

What do we do with a model engine??
How we differentiate model engine from the plain vector engine (ex: Hnsw)??

sorry i do not understand this question.

Why a model engine does not need a resource clean up in cache manager when DVReader is being closed??

i see there are 3 memory allocation IndexAllocation, TrainingDataAllocation, AnonymousAllocation. cache manager do not mange all of these?

[0ctopus13prime]

@luyuncheng
Thank you, but it still does not answer the questions. :)
Overall, I'm trying to understand what 'model engine' is. And how it's different from a plain graph based vector engine like HNSW.

Could you give more contexts why did you add the logic so that those 'model engines' are excluded from invalidating the cache??

    private KNNEngine getNativeKNNEngine(@NonNull FieldInfo field) {
        final String modelId = field.attributes().get(MODEL_ID);
        if (modelId != null) { <------------ This one. Why did you add it?
            return null;
        }
        KNNEngine engine = FieldInfoExtractor.extractKNNEngine(field);
        if (KNNEngine.getEnginesThatCreateCustomSegmentFiles().contains(engine)) {
            return engine;
        }
        return null;
    }

[0ctopus13prime]

Hi @luyuncheng,
could you answer ^^ questions when you have time?

What I understand is modelId coming from cluster state also I confirmed OpneSearch build its index internally.
I recently removed the code so that allocated memory can be released after we deprecated FileWatcher.
I think this is safe to do that, but hoping to get your motivation for the if-branch just in case.

Thank you