Allow low level paging in LeafDocLookup

[rjernst]

Accessing a field from a script may require lots of low level operations for which scripts do not have access. Initial loading of doc values allows this access with appropriate doPrivileged calls, but advancing to the curent document does not. This commit also protects advancing docs in the same way.

[elasticsearchmachine]

Pinging @elastic/es-core-infra (Team:Core/Infra)

[elasticsearchmachine]

Hi @rjernst, I've created a changelog YAML for you.

[stu-elastic]

It would be nice if we could somehow test this, but that may have to be an integ test

[stu-elastic]

Is this AccessibleObject import necessary?

[rjernst]

Nope, accidentally imported, removed now

[stu-elastic]

I wonder if you could add a script field to a yamlRestTest in the searchable-snapshots x-pack plugin? I can't get the setNextDocId to trigger the security exception yet, maybe we need multiple shards?

[stu-elastic]

lgtm

[iverase]

I think this change has introduced an important performance regression. Here is how the flamegraph of a vector tiles API calls look like after this change:

The API is executing a search and sorting the data using a script. It is running over ~13 million documents. Could we do something to mitigate the performance penalty here?

[rjernst]

@ChrisHegarty do you have thoughts on how we could do these doPriv calls differently? I had thought doPriv should not have a performance penalty

[ChrisHegarty]

I assume that the java.security.AccessController.isPrivileged is the notable point to take from the above flame graph? This is kinda odd, since 1) I don't immediately see how it relates to the changes in the PR, and 2) that method, isPrivileged, is a JDK method that is only invoked from assert statements. Am I interpreting the above flame graph correctly ? (is inlining effectively flattening some stack frames in the flame graph )

Additionally, I would try making the advanceToDoc method static and passing the docId (rather than a virtual method). and maybe try playing a little with PrivilegedExceptionAction. The suspicion, without any concrete foundation, is that maybe it is not being inlined.

[iverase]

This is how the stack looks like:

[ChrisHegarty]

Does the benchmark run with JDK system assertions enabled? (since the JDK code seems to only execute isPrivileged if this is the case)

[iverase]

I run Elasticsearch locally and pass the following jvm argument: -Dtests.jvm.argline="-da", that should disable assertions? In addition, it does not explain the effect on nightly benchmarks?

Here is the full command:

./gradlew run -Drun.license_type=trial --data-dir=path-to-index -Dtests.heap.size=4G -Dtests.es.xpack.security.enabled=false -Dtests.es.http.cors.enabled=true -Dtests.es.http.cors.allow-origin='"*"' -Dtests.es.xpack.ml.enabled=false -Dtests.jvm.argline="-da"

[ChrisHegarty]

I run Elasticsearch locally and pass the following jvm argument: -Dtests.jvm.argline="-da", that should disable assertions?

JDK assertions and user-level assertions are handled differently, e.g. the JDK assertions are disabled by:
-disablesystemassertions or -dsa

In addition, it does not explain the effect on nightly benchmarks?

Right. I'm not drawing any conclusions yet, just trying to understand the stacks here (which still don't make full sense to me).

[iverase]

JDK assertions and user-level assertions are handled differently, e.g. the JDK assertions are disabled by:
-disablesystemassertions or -dsa

TIL! That explain what I am seeing.

[iverase]

Indeed that removes the issue in my case:

[iverase]

I open #93971 to update the testing instructions.

[iverase]

One thing I noticed is that we seem to be allocating lambda instances on the method, I guess a symptom of not being inlined:

[iverase]

My guess is that the issue here is that docId is not final so the virtual method does not get inlined. Would that copy the docId to a final variable fix the issue?

 private void advanceToDoc(DocValuesScriptFieldFactory factory) {
        // advancing to the current doc could trigger low level paging, network loading, etc, so do so outside scripting privileges
        // copy the docId to a final variable so the virtual method below get inlined.
        final int tempDocId = this.docId;
        AccessController.doPrivileged((PrivilegedAction<Void>) () -> {
            try {
                factory.setNextDocId(tempDocId);
            } catch (IOException ioe) {
                throw ExceptionsHelper.convertToElastic(ioe);
            }
            return null;
        });
    }

[ChrisHegarty]

Yeah. Not quite inlining, but there is a less-than-ideal situation. The implementation results in a capturing lambda, which captures (among other things) the LeafDocLookup - so a lambda instance is created for each and every LeafDocLookup lookup. This can be seen in the below debug trace (when stopped at a breakpoint).

Notice:

1. The lambda implementation class constructor takes two args, the LeafDocLookup and the factory
2. The lambda instance stores these ctr args as final fields.

[ChrisHegarty]

My guess is that the issue here is that docId is not final so the virtual method does not get inlined. Would that copy the docId to a final variable fix the issue?

This is better, but take a look at the lambda after this change. I suspect that the constructor params has just changed to factory and int docId ? ( still many instances? one instance per LeafDocLookup instance ? )

[ChrisHegarty]

@iverase your suggestion, to store the docId in a local, is better than the current code (since the whole LeafDocLookup is not captured), but may not prevent the lambda instance creation. Alternatively, the advanceToDoc method could be made static, and pass the docId as an arg. Still the lambda instances will be created :-(

[ChrisHegarty]

@iverase Still worth experimenting with though. It is possible that some simplifications, like the ones above, will help with the JIT inlining and/or escape analysis.

[iverase]

I guess we can always wrap the DocValuesScriptFieldFactory. won't be pretty but we will not be making an allocation per docId.

   private static class DocValuesScriptFieldFactoryWrapper implements PrivilegedAction<Void> {
        final DocValuesScriptFieldFactory factory;
        int docId;

        DocValuesScriptFieldFactoryWrapper(DocValuesScriptFieldFactory factory) {
            this.factory = factory;
        }

        public Void run() {
            try {
                factory.setNextDocId(docId);
            } catch (IOException ioe) {
                throw ExceptionsHelper.convertToElastic(ioe);
            }
            return null;
        }
    }