javadelight / delight-nashorn-sandbox

A sandbox for executing JavaScript with Nashorn in Java.
Other
268 stars 81 forks source link

A few suggestions for sandbox optimization, and I've practiced it on flink's high concurrency performance. #126

Open MrLi2018 opened 2 years ago

MrLi2018 commented 2 years ago

Hello, MXRO, I can provide several optimization ideas, and I have accumulated experience in practice. 1、For example, by removing the interface implementation of Runnable of JsEvaluator and executing only the run method in executeSandboxedOperation, I have tested that the performance is improved by about 50% and the sandbox can avoid throwing IllegalStateException. If the sandbox resource limit is exceeded, an exception is directly thrown instead of using the stop method to forcibly stop the thread. Is it better to throw an exception and leave the exception to the caller for processing? 2、It is recommended that a unified script execution interface be provided for the sandbox so that a Java interface object can be generated and cached by implementing the getInterface method in Invocable. In the next execution, jsBean.run(Object params) can be directly read from the cache and directly executed.

MrLi2018 commented 2 years ago

I'm willing to contribute code if I can, because I love the nashorn sandbox.

mxro commented 2 years ago

Thank you for raising this issue!

That all sounds like good ideas to me, be welcome to contribute an implementation for them 😊

MrLi2018 commented 2 years ago

I'm just a suggestion, and I'm not sure if I can change to synchronous execution instead of using thread pools.

MrLi2018 commented 2 years ago

Can we use limiting the number of cycles or recursions to replace limiting the CPU to improve script execution performance? For example, add the following injection code: public static void inject() throws InterruptedException { ScriptEngineUtils.logCall(); if (Thread.interrupted()) { throw new JsInterruptException(); } }

public final class ScriptEngineUtils { private ScriptEngineUtils() { }

private static ThreadLocal<AtomicInteger> callNumber = new ThreadLocal<>();

private static final Integer MAX_INVOKE_COUNT = 100000;

public static void initInvokeCount() {
    callNumber.set(new AtomicInteger(0));
}

public static void removeInvokeCount() {
    if (ObjectUtils.isNotEmpty(callNumber)) {
        callNumber.remove();
    }
}

public static void logCall() {
    AtomicInteger index = callNumber.get();
    if (ObjectUtils.isNotEmpty(index)) {
        int count = index.getAndIncrement();
        if (count > runMaxCount) {
            removeInvokeCount();
            throw new JsInterruptException("Script Detected deadloop.");
        }
    } else {
        callNumber.set(new AtomicInteger(0));
    }

}

} Remove threadloacl from subthreads and add a field to limit the number of cycles at the monitoring layer. For example: if (isCpuTimeExided(runtime) || isMemoryLimitExceeded()|| cycleMaxNumLimitExceeded()) {

                    cpuLimitExceeded.set(isCpuTimeExided(runtime));
                    cycleMaxNumLimitExceeded.set(true);
                    threadToMonitor.interrupt();
                    synchronized (monitor) {
                        //wait less
                        monitor.wait(5);
                    }
                    if (stop.get()) {
                        return;
                    }
                    if (!scriptFinished.get()) {
                        threadToMonitor.stop();
                        scriptKilled.set(true);
                    }
                    return;

} I don't know if it's possible to use this method to replace the CPU limit.

mxro commented 2 years ago

Thanks for the ideas! Could you put them into a PR - maybe easier to see/discuss there! Thank you!!

MrLi2018 commented 2 years ago

Okay, thanks. I'll try, github. I'm not familiar with it yet.