GoogleCloudPlatform / functions-framework-java

FaaS (Function as a service) framework for writing portable Java functions
Apache License 2.0
130 stars 63 forks source link

Port to use Jetty-12 core without servlets #235

Open gregw opened 10 months ago

gregw commented 10 months ago

This is port of the functions framework to use jetty-12 core APIs.

conventional-commit-lint-gcf[bot] commented 10 months ago

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot https://conventionalcommits.org/

gregw commented 10 months ago

The core APIs means that the overheads of the servlet API can be avoided. However, there a number of conveniences that are provided in the servlet implementation that are not easily accessed from the core APIs (eg Charset handling). So this branch is currently build against a SNAPSHOT, as I'm updating jetty to better support this use case.

So this is rather premature to open a PR, but I just wanted to make a place holder to show the effort is underway.

Probably biggest future hurdle is that jetty-12 requires JDK 17.

Future work I'd like to look at is to better support the asynchronous features of Jetty. E.g. we can asynchronously load the multi-parts of a request before we use a blocking thread to invoke the function.

ludoch commented 10 months ago

Hi Greg, thanks for doing this. The function framework today is currently shared between Java11 and Java17 runtime targets, and jetty12 does not support java11! The java11 target will be declared end of support in October 2024 I think, so we need to keep java11 until then. Once this is done, we could upgrade java17 and java21 to Jetty12 and drop entirely java11 target. Or we could immediately also support java21 with a dual target: jetty12 or jetty9 for java11 (with some class loading magic like we are doing for GAE) and if the runtime target is java21, use the jetty12 implementation, else use the current jetty9 version. We've asked for extended jetty9 support on java21 until October 2024, that would be the other option to enable FF java21 now. WDYT? Anyway, as such, the PR will not work with the current java11 Function invoker target which is still useable in prod for customers.

gregw commented 10 months ago

@ludoch Jetty 9 support will continue whilst there are commercial support clients, so October 2024 is not an technical issue.

I've gone for the port to Jetty-12 now for a number of reasons:

Can you explain a bit more why the functions currently deployed to java11 cannot be moved to a java17 runtime? The functions compiled in java11 will still run in java17 and there should be no differences in the behaviour that they see. It is not like the full java runtime, where there are servlet changes between the runtimes. Is the issue more about how those functions are deployed?

Note that this PR has a green build locally against the SNAPSHOT build of Jetty-12

gregw commented 10 months ago

The latest commits have been aimed at improving efficiency. Specifically by moving the buffering below the OutputStream, then we are able to commit responses that are less than the buffer size in a single operation, avoiding the need to use HTTP/1.1 chunking to frame the response. This currently is a green against the latest snapshot of jetty-12 and should would against the soon to be released jetty-12.0.2

gregw commented 10 months ago

@ludoch note that I could fairly easily make a lot of the optimizations from this branch in the main branch as well. There is actually no need to use Servlets just for a bit of output stream wrapping etc. The improvements would not be as much as we can achieve with 12, but they would probably be noticeable.

gregw commented 3 months ago

See #241 that removes Servlets from the current jetty-9 usage, which should give reasonable savings in memory and CPU. See #244 That upgrades to Jetty-10 for a more modern/efficient HTTP server, which should give a bit more CPU savings. This PR will be revived once JDK17 is the minimal supported version and this will give even more memory and CPU savings.