Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port to use Jetty-12 core without servlets #235

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

gregw
Copy link

@gregw gregw commented Sep 22, 2023

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

@conventional-commit-lint-gcf
Copy link

🤖 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
Copy link
Author

gregw commented Sep 22, 2023

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
Copy link
Collaborator

ludoch commented Sep 22, 2023

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
Copy link
Author

gregw commented Sep 26, 2023

@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:

  • The jetty-core APIs are new and I wanted to check they were easily usable for an usage like web functions. It turned out it mostly was, but there were a few things like charset determinations and efficient OutputStreamWriters that were not as easy as using servlets. I have a PR for jetty to fix this and that should be in 12.0.2 (or 12.0.3).
  • Avoiding the overhead of Servlets should be a performance benefit in both latency and memory footprint, plus Jetty-12 in generally faster anyway. So it would be good to see if this branch could be benchmarked against the existing java11/jetty-9 to see what improvements there are.
  • This web functions usage is ideally suited to some asynchronous speed up. Specifically for things like request multi-part or web forms, we can easily put a handler in place that will asynchronously read all the request content before invoking the function in blocking mode. This will mean that the function will never block in getParts() or similar, which should reduce thread usage and memory footprint for such functions.

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

Port the invoker to upgrade to Eclipse Jetty-12 version 12.  Specifically using the new core APIs of Eclipse Jetty-12 that allow the overhead of a Servlet container to be avoided.

BREAKING CHANGE: use Java 17 or above, as required by Eclipse Jetty-12.
Port the invoker to upgrade to Eclipse Jetty-12 version 12.  Specifically using the new core APIs of Eclipse Jetty-12 that allow the overhead of a Servlet container to be avoided.

BREAKING CHANGE: use Java 17 or above, as required by Eclipse Jetty-12.
Port the invoker to upgrade to Eclipse Jetty-12 version 12.  Specifically using the new core APIs of Eclipse Jetty-12 that allow the overhead of a Servlet container to be avoided.   Several other minor optimizations are included.

BREAKING CHANGE: use Java 17 or above, as required by Eclipse Jetty-12.
Port the invoker to upgrade to Eclipse Jetty-12 version 12.  Specifically using the new core APIs of Eclipse Jetty-12 that allow the overhead of a Servlet container to be avoided.   Several optimizations are included.

BREAKING CHANGE: use Java 17 or above, as required by Eclipse Jetty-12.
Port the invoker to upgrade to Eclipse Jetty-12 version 12.  Specifically using the new core APIs of Eclipse Jetty-12 that allow the overhead of a Servlet container to be avoided.   Several optimizations are included, including the optimization that small writes can be buffered in such a way to avoid chunked responses when possible.

BREAKING CHANGE: use Java 17 or above, as required by Eclipse Jetty-12.
Port the invoker to upgrade to Eclipse Jetty-12 version 12.  Specifically using the new core APIs of Eclipse Jetty-12 that allow the overhead of a Servlet container to be avoided.   Several optimizations are included, including the optimization that small writes can be buffered in such a way to avoid chunked responses when possible.

BREAKING CHANGE: use Java 17 or above, as required by Eclipse Jetty-12.
@gregw
Copy link
Author

gregw commented Sep 26, 2023

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
Copy link
Author

gregw commented Sep 27, 2023

@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.

Port the invoker to upgrade to Eclipse Jetty-12 version 12.  Specifically using the new core APIs of Eclipse Jetty-12 that allow the overhead of a Servlet container to be avoided.   Several optimizations are included, including the optimization that small writes can be buffered in such a way to avoid chunked responses when possible.

BREAKING CHANGE: use Java 17 or above, as required by Eclipse Jetty-12.
@gregw
Copy link
Author

gregw commented Apr 18, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants