Possible performance impact due com.vaadin.mpr.MprServlet detection

[rPraml]

Affected version: Vaadin Framework 8.19.0

Hello, I compared the vaadin-server source changes 8.18.0 <-> 8.19.0 and noticed, that there are mostly changes, that look like this:

    private boolean assertConnector(ClientConnector connector) {
        try {
            Class.forName("com.vaadin.mpr.MprServlet", false,
                    getClass().getClassLoader());
            return true;
        } catch (ClassNotFoundException e) {
            return connectorIdToConnector
                    .get(connector.getConnectorId()) == connector;
        }
    }

I'm afraid, that this could cause a massive performance impact, if the MprServlet is not on the classpath, as every time an exception is generated and thrown.
I see that this code may be executed multiple times per request. I see also that most code paths are only executed, if assertions are enabled, but disabling assertions is not really an option.

I would suggest to cache the result, if MprServlet is present or not.

Please see this test about performance results and a possible fix

public class TestPerformance {

	private boolean isMprPresent() {
		try {
			Class.forName("com.vaadin.mpr.MprServlet", false, getClass().getClassLoader());
			return true;
		} catch (ClassNotFoundException e) {
			return false;
		}
	}

	private boolean isUiPresent() {
		try {
			Class.forName("com.vaadin.ui.UI", false, getClass().getClassLoader());
			return true;
		} catch (ClassNotFoundException e) {
			return false;
		}
	}

	/**
	 * The com.vaadin.ui is present. This test runs in about 200ms.
	 */
	@Test
	void testUiPresent() {
		for (int i = 0; i < 1_000_000; i++) {
			assertThat(isUiPresent()).isTrue();
		}
	}

	/**
	 * The com.vaadin.mpr.MprServlet is not present, so we have to generate a ClassNotFound exception every time. 
	 * This test runs in about 27 seconds. This is more than 100 times slower.
	 */
	@Test
	void testMprPresent() {
		for (int i = 0; i < 1_000_000; i++) {
			assertThat(isMprPresent()).isFalse();
		}
	}

	/**
	 * This is an better way I suggest to cache, if MprServlet is present. This test runs in about 150ms.
	 */
	@Test
	void testMprPresentFast() {
		for (int i = 0; i < 1_000_000; i++) {
			assertThat(MprCheck.isPresent()).isFalse();
		}
	}

	private static class MprCheck {
		private static boolean present;

		static {
			try {
				Class.forName("com.vaadin.mpr.MprServlet", false, MprCheck.class.getClassLoader());
				present = true;
			} catch (ClassNotFoundException e) {
				present = false;
			}
		}

		public static boolean isPresent() {
			return present;
		}
	}
}

cheers
Roland

[TatuLund]

I am pleased to see that our are able to detect details like this. This check is done only assertions (as we are doing many other checkings that can have performance impact), so this should not be a concern.

[TatuLund]

, if assertions are enabled, but disabling assertions is not really an option.

Do you mean that you run assertions enabled in production?

[rPraml]

Hello Tatu,
currently, we have enabled assertions in production. (and that's good - we are still investigating why #12565 happens).
I know that enabling assertions may affect performance.
We do not have vaadin 8.19 productive, yet. So I cannot say how the overall performance is affected by this particular code change.
I only did a quick review what's changed between 8.18 and 8.19 and noticed, that there is a possible performance bottleneck, which can be easily fixed.

[TatuLund]

Improvement will be included in Vaadin 8.19.1