revert #11482 and fix redundant code generation

[colinsurprenant]

Fixes #11560

The fix for the Java execution slowdown relative to the number of worker in #11482 introduced a regression when using multiple pipelines. By analyzing the regression we realized that although the fix was solving the problem for the single pipeline with multiple workers it actually did not correctly solve the root cause of the problem. By removing the global class cache it prevented cross-pipelines class caching, which resulted in the regression.

This PR reverts the previous fix and correctly address the root cause. The real problem was that the configuration Java code was being generated multiple times through the hashCode() and equals() methods which were calling the normalizedSource() method multiple times. The fix addresses this by memoizing the hashCode() and normalizedSource() methods. A few other optimizations have also been made.

The result is that the multiple worker slowdown is still solved and the regression is also fixed.

[yaauie]

I think this is the right approach for now, to get a fix for both scenarios out the door in a timely manner, but agree with your other comments that we should take a step back and reevaluate code generation in general in a separate effort.

I've left a note about maybe getting instantiation out of the global lock, and another requesting we keep or enhance commentary about the need for synchronization.

[colinsurprenant]

• I revisited the need to have the instantiation inside the compile lock and I did not understand why it was required probably for good reasons because it does not look like it is required. My previous tests for that probably included other changes which created other concurrency issues.

• As it is now, using the synchronized (COMPILER) is basically equivalent to just synchronizing the method.

• Now that we know that instantiation does not need to be in that synchronization, I tried using a ReadWriteLock, the logic being that multiple threads should be able to concurrently read the cache for already compiled classes. The problem we still have is that the write side of actually performing the compilation is disproportionate to the read side, so every time a write/compilation is performed, everything is locked.

Here's a test implementation of using a ReentrantReadWriteLock, which follows the Javadoc sample usage example.

    private  Class<? extends Dataset> compile() {
        try {
            COMPILE_LOCK.readLock().lock();

            Class<? extends Dataset> clazz = CLASS_CACHE.get(this);
             if (clazz == null) {
                // must release read lock before acquiring write lock
                COMPILE_LOCK.readLock().unlock();
                COMPILE_LOCK.writeLock().lock();
                try {
                    // recheck state because another thread might have
                    // acquired write lock and changed state before we did.
                    clazz = CLASS_CACHE.get(this);
                    if (clazz == null) {
                        final String name = String.format("CompiledDataset%d", CLASS_CACHE.size());
                        final String code = CLASS_NAME_PLACEHOLDER_REGEX.matcher(normalizedSource).replaceAll(name);
                        if (SOURCE_DIR != null) {
                            final Path sourceFile = SOURCE_DIR.resolve(String.format("%s.java", name));
                            Files.write(sourceFile, code.getBytes(StandardCharsets.UTF_8));
                            COMPILER.cookFile(sourceFile.toFile());
                        } else {
                            COMPILER.cook(code);
                        }
                        COMPILER.setParentClassLoader(COMPILER.getClassLoader());
                        clazz = (Class<T>) COMPILER.getClassLoader().loadClass(
                                String.format("org.logstash.generated.%s", name)
                        );
                        CLASS_CACHE.put(this, clazz);
                    }
                    // downgrade by acquiring read lock before releasing write lock
                    COMPILE_LOCK.readLock().lock();
                } finally {
                    // unlock write, still hold read
                    COMPILE_LOCK.writeLock().unlock();
                }
            }
            COMPILE_LOCK.readLock().unlock();
            return clazz;
        } catch (final CompileException | ClassNotFoundException | IOException ex) {
            throw new IllegalStateException(ex);
        }
    }

So far it seems to be working correctly but I have not seen any significant performance difference with my local tests. WDYT?

I will try a variation where the actual compilation will happen outside the write lock, which will help not holding the write lock for too long at the potential cost of performing multiple compilations.

[colinsurprenant]

@yaauie I think we should move on with the current simpler and straightforward synchronization (pending LGTM) for the sake of moving forward with a correct solution with solves the regression to unblock 7.6. Further optimization work can be worked on in another PR.

[yaauie]

+1 to merging as-is and chasing down locking optimization in a separate effort.

[colinsurprenant]

Merging in master, backports for 7.7, 7.6, 7.5.3